* [PATCH 0/2] v4l: vsp1: Add HGT support @ 2016-09-02 13:47 Niklas Söderlund 2016-09-02 13:47 ` [PATCH 1/2] v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine Niklas Söderlund 2016-09-02 13:47 ` [PATCH 2/2] v4l: vsp1: Add HGT support Niklas Söderlund 0 siblings, 2 replies; 9+ messages in thread From: Niklas Söderlund @ 2016-09-02 13:47 UTC (permalink / raw) To: linux-renesas-soc, linux-media, laurent.pinchart Cc: corbet, mchehab, sakari.ailus, hans.verkuil, Niklas Söderlund Hi, This series add support for the VSP1 2-D histogram engine HGT. It's based on top of Laurent Pinchart tree at git://linuxtv.org/pinchartl/media.git hgo. It is tested on Koelsch using a modified vsp-tests suite package, modifications can be found at https://git.ragnatech.se/vsp-tests hgt. Niklas Söderlund (2): v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine v4l: vsp1: Add HGT support Documentation/media/uapi/v4l/meta-formats.rst | 1 + .../media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst | 150 +++++++ drivers/media/platform/vsp1/Makefile | 2 +- drivers/media/platform/vsp1/vsp1.h | 3 + drivers/media/platform/vsp1/vsp1_drv.c | 32 +- drivers/media/platform/vsp1/vsp1_entity.c | 33 +- drivers/media/platform/vsp1/vsp1_entity.h | 1 + drivers/media/platform/vsp1/vsp1_hgt.c | 495 +++++++++++++++++++++ drivers/media/platform/vsp1/vsp1_hgt.h | 51 +++ drivers/media/platform/vsp1/vsp1_pipe.c | 16 + drivers/media/platform/vsp1/vsp1_pipe.h | 2 + drivers/media/platform/vsp1/vsp1_regs.h | 9 + drivers/media/platform/vsp1/vsp1_video.c | 10 +- drivers/media/v4l2-core/v4l2-ioctl.c | 1 + include/uapi/linux/videodev2.h | 3 +- 15 files changed, 792 insertions(+), 17 deletions(-) create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h -- 2.9.3 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine 2016-09-02 13:47 [PATCH 0/2] v4l: vsp1: Add HGT support Niklas Söderlund @ 2016-09-02 13:47 ` Niklas Söderlund 2016-09-05 13:05 ` Laurent Pinchart 2016-09-02 13:47 ` [PATCH 2/2] v4l: vsp1: Add HGT support Niklas Söderlund 1 sibling, 1 reply; 9+ messages in thread From: Niklas Söderlund @ 2016-09-02 13:47 UTC (permalink / raw) To: linux-renesas-soc, linux-media, laurent.pinchart Cc: corbet, mchehab, sakari.ailus, hans.verkuil, Niklas Söderlund The format is used on the R-Car VSP1 video queues that carry 2-D histogram statistics data. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- Documentation/media/uapi/v4l/meta-formats.rst | 1 + .../media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst | 150 +++++++++++++++++++++ drivers/media/v4l2-core/v4l2-ioctl.c | 1 + include/uapi/linux/videodev2.h | 3 +- 4 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst diff --git a/Documentation/media/uapi/v4l/meta-formats.rst b/Documentation/media/uapi/v4l/meta-formats.rst index 05ab91e..01e24e3 100644 --- a/Documentation/media/uapi/v4l/meta-formats.rst +++ b/Documentation/media/uapi/v4l/meta-formats.rst @@ -13,3 +13,4 @@ These formats are used for the :ref:`metadata` interface only. :maxdepth: 1 pixfmt-meta-vsp1-hgo + pixfmt-meta-vsp1-hgt diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst new file mode 100644 index 0000000..a093f0a --- /dev/null +++ b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst @@ -0,0 +1,150 @@ +.. -*- coding: utf-8; mode: rst -*- + +.. _v4l2-meta-fmt-vsp1-hgt: + +******************************* +V4L2_META_FMT_VSP1_HGT ('VSPT') +******************************* + +*man V4L2_META_FMT_VSP1_HGT(2)* + +Renesas R-Car VSP1 2-D Histogram Data + + +Description +=========== + +This format describes histogram data generated by the Renesas R-Car VSP1 +2-D Histogram (HGT) engine. + +The VSP1 HGT is a histogram computation engine that operates on HSV +data. It operates on a possibly cropped and subsampled input image and +computes the sum, maximum and minimum of the S component as well as a +weighted frequency histogram based on the H and S components. + +The histogram is a matrix of 6 Hue and 32 Saturation buckets, 192 in +total. Each HSV value is added to one or more buckets with a wight +between 1 and 16 depending on how the Hue areas are setup. Finding the +correct buckets is done by inspecting the H and S value independently. + +The Saturation position **n** (0 - 31) in the matrix are found by the +expression: + + 8 * n <= S < 8 * (n + 1) + +The Hue positions **m** (0 - 5) in the matrix depends on how the HGT Hue +areas are configured. There are 6 user configurable Hue Areas which can +be configured to cover overlapping Hue values: + +:: + + Area 0 Area 1 Area 2 Area 3 Area 4 Area 5 + ________ ________ ________ ________ ________ ________ + \ /| |\ /| |\ /| |\ /| |\ /| |\ /| |\ / + \ / | | \ / | | \ / | | \ / | | \ / | | \ / | | \ / + X | | X | | X | | X | | X | | X | | X + / \ | | / \ | | / \ | | / \ | | / \ | | / \ | | / \ + / \| |/ \| |/ \| |/ \| |/ \| |/ \| |/ \ + 5U 0L 0U 1L 1U 2L 2U 3L 3U 4L 4U 5L 5U 0L + <0..............................Hue Value............................255> + +As shown in the diagram a single Hue vale can be attributed to more then +one Hue area. In such case the Hue value is attributed to both Hue +Areas, but with a weight. The maximum weight is 16 and is associated +with all Hue values that are inside the center of a Hue area (between nL +-- nU). Values outside this area are weighted with a rounded down value +along the diagonal line. If there is no overlapped areas specified the +value is included in the lower area. + +The Hue area setup must match one of the following constrains: + +:: + + 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U + +:: + + 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L + +**Byte Order.** +All data is stored in memory in little endian format. Each cell in the tables +contains one byte. + +.. flat-table:: VSP1 HGT Data - (800 bytes) + :header-rows: 2 + :stub-columns: 0 + + * - Offset + - :cspan:`4` Memory + * - + - [31:24] + - [23:16] + - [15:8] + - [7:0] + * - 0 + - - + - S max [7:0] + - - + - S min [7:0] + * - 4 + - :cspan:`4` S sum [31:0] + * - 8 + - - + - Hue Area 0 Lower Boundary (0L) [0:7] + - - + - Hue Area 0 Upper Boundary (0U) [0:7] + * - 12 + - - + - Hue Area 1 Lower Boundary (1L) [0:7] + - - + - Hue Area 1 Upper Boundary (1U) [0:7] + * - 16 + - - + - Hue Area 2 Lower Boundary (2L) [0:7] + - - + - Hue Area 2 Upper Boundary (2U) [0:7] + * - 20 + - - + - Hue Area 3 Lower Boundary (3L) [0:7] + - - + - Hue Area 3 Upper Boundary (3U) [0:7] + * - 24 + - - + - Hue Area 4 Lower Boundary (4L) [0:7] + - - + - Hue Area 4 Upper Boundary (4U) [0:7] + * - 28 + - - + - Hue Area 5 Lower Boundary (5L) [0:7] + - - + - Hue Area 5 Upper Boundary (5U) [0:7] + * - 32 + - :cspan:`4` Histogram bucket (m=0, n=0) [31:0] + * - 36 + - :cspan:`4` Histogram bucket (m=0, n=1) [31:0] + * - + - :cspan:`4` ... + * - 156 + - :cspan:`4` Histogram bucket (m=0, n=31) [31:0] + * - 160 + - :cspan:`4` Histogram bucket (m=1, n=0) [31:0] + * - + - :cspan:`4` ... + * - 288 + - :cspan:`4` Histogram bucket (m=2, n=0) [31:0] + * - + - :cspan:`4` ... + * - 416 + - :cspan:`4` Histogram bucket (m=3, n=0) [31:0] + * - + - :cspan:`4` ... + * - 544 + - :cspan:`4` Histogram bucket (m=4, n=0) [31:0] + * - + - :cspan:`4` ... + * - 672 + - :cspan:`4` Histogram bucket (m=5, n=0) [31:0] + * - + - :cspan:`4` ... + * - 796 + - :cspan:`4` Histogram bucket (m=5, n=31) [31:0] diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index b7f7d5f..f459c4f 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -1259,6 +1259,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) case V4L2_SDR_FMT_CS14LE: descr = "Complex S14LE"; break; case V4L2_SDR_FMT_RU12LE: descr = "Real U12LE"; break; case V4L2_META_FMT_VSP1_HGO: descr = "R-Car VSP1 1-D Histogram"; break; + case V4L2_META_FMT_VSP1_HGT: descr = "R-Car VSP1 2-D Histogram"; break; default: /* Compressed formats */ diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 1dbe52a..c8c046c 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -638,7 +638,8 @@ struct v4l2_pix_format { #define V4L2_SDR_FMT_RU12LE v4l2_fourcc('R', 'U', '1', '2') /* real u12le */ /* Meta-data formats */ -#define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car VSP1 Histogram */ +#define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car VSP1 1-D Histogram */ +#define V4L2_META_FMT_VSP1_HGT v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */ /* priv field value to indicates that subsequent fields are valid. */ #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe -- 2.9.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine 2016-09-02 13:47 ` [PATCH 1/2] v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine Niklas Söderlund @ 2016-09-05 13:05 ` Laurent Pinchart 2016-09-05 13:34 ` Niklas Söderlund 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2016-09-05 13:05 UTC (permalink / raw) To: Niklas Söderlund Cc: linux-renesas-soc, linux-media, corbet, mchehab, sakari.ailus, hans.verkuil Hi Niklas, Thank you for the patch. On Friday 02 Sep 2016 15:47:13 Niklas Söderlund wrote: > The format is used on the R-Car VSP1 video queues that carry > 2-D histogram statistics data. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > Documentation/media/uapi/v4l/meta-formats.rst | 1 + > .../media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst | 150 ++++++++++++++++++ > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > include/uapi/linux/videodev2.h | 3 +- > 4 files changed, 154 insertions(+), 1 deletion(-) > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst [snip] > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst > b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst new file mode > 100644 > index 0000000..a093f0a > --- /dev/null > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst > @@ -0,0 +1,150 @@ > +.. -*- coding: utf-8; mode: rst -*- > + > +.. _v4l2-meta-fmt-vsp1-hgt: > + > +******************************* > +V4L2_META_FMT_VSP1_HGT ('VSPT') > +******************************* > + > +*man V4L2_META_FMT_VSP1_HGT(2)* > + > +Renesas R-Car VSP1 2-D Histogram Data > + > + > +Description > +=========== > + > +This format describes histogram data generated by the Renesas R-Car VSP1 > +2-D Histogram (HGT) engine. > + > +The VSP1 HGT is a histogram computation engine that operates on HSV > +data. It operates on a possibly cropped and subsampled input image and > +computes the sum, maximum and minimum of the S component as well as a > +weighted frequency histogram based on the H and S components. > + > +The histogram is a matrix of 6 Hue and 32 Saturation buckets, 192 in > +total. Each HSV value is added to one or more buckets with a wight s/wight/weight/ > +between 1 and 16 depending on how the Hue areas are setup. I would say 'depending on the Hue areas configuration' to insist on the fact that the configuration can be changed by the application. > Finding the > +correct buckets is done by inspecting the H and S value independently. Maybe s/correct/corresponding/ ? > +The Saturation position **n** (0 - 31) in the matrix are found by the Maybe s/in the matrix/of the bucket/, or s/in the matrix/of the bucket in the matrix/ ? s/are found/is found/ or maybe 'is computed' ? > +expression: > + > + 8 * n <= S < 8 * (n + 1) How about simply 'n = S / 8' ? > +The Hue positions **m** (0 - 5) in the matrix depends on how the HGT Hue s/positions/position/ > +areas are configured. There are 6 user configurable Hue Areas which can > +be configured to cover overlapping Hue values: > + > +:: > + > + Area 0 Area 1 Area 2 Area 3 Area 4 > Area 5 + ________ ________ ________ ________ > ________ ________ + \ /| |\ /| |\ /| |\ /| > |\ /| |\ /| |\ / + \ / | | \ / | | \ / | > | \ / | | \ / | | \ / | | \ / + X | | X | | > X | | X | | X | | X | | X + / \ | | / > \ | | / \ | | / \ | | / \ | | / \ | | / \ + / > \| |/ \| |/ \| |/ \| |/ \| |/ \| |/ > \ + 5U 0L 0U 1L 1U 2L 2U 3L 3U 4L 4U > 5L 5U 0L + <0..............................Hue > Value............................255> > + > +As shown in the diagram a single Hue vale can be attributed to more then s/vale/value/ s/then/than/ > +one Hue area. In such case the Hue value is attributed to both Hue > +Areas, but with a weight. The maximum weight is 16 and is associated > +with all Hue values that are inside the center of a Hue area (between nL > +-- nU). Values outside this area are weighted with a rounded down value > +along the diagonal line. If there is no overlapped areas specified the > +value is included in the lower area. This sounds a bit confusing to me. How about the following ? "The Hue position **m** (0 - 5) of the bucket [in the matrix]* depends on how the HGT Hue areas are configured. There are 6 user configurable Hue Areas which can be configured to cover overlapping Hue values: [diagram] When two consecutive areas don't overlap (n+1L is equal to nU) the boundary value is considered as part of the lower area. Pixels with a hue value included in the centre of an area (between nL and nU included) are are attributed to that single area and given a weight of 16. Pixels with a hue value included in the overlapping region between two areas (between n+1L and nU excluded) are attributed to both areas and given a weight for each of these areas proportional to their position along the diagonal lines (rounded down)." * Add "in the matrix" depending on the wording of the saturation description. > +The Hue area setup must match one of the following constrains: > + > +:: > + > + 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U > + > +:: > + > + 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L > + > +**Byte Order.** > +All data is stored in memory in little endian format. Each cell in the > tables +contains one byte. > + > +.. flat-table:: VSP1 HGT Data - (800 bytes) > + :header-rows: 2 > + :stub-columns: 0 > + > + * - Offset > + - :cspan:`4` Memory > + * - > + - [31:24] > + - [23:16] > + - [15:8] > + - [7:0] > + * - 0 > + - - > + - S max [7:0] > + - - > + - S min [7:0] > + * - 4 > + - :cspan:`4` S sum [31:0] > + * - 8 > + - - > + - Hue Area 0 Lower Boundary (0L) [0:7] > + - - > + - Hue Area 0 Upper Boundary (0U) [0:7] > + * - 12 > + - - > + - Hue Area 1 Lower Boundary (1L) [0:7] > + - - > + - Hue Area 1 Upper Boundary (1U) [0:7] > + * - 16 > + - - > + - Hue Area 2 Lower Boundary (2L) [0:7] > + - - > + - Hue Area 2 Upper Boundary (2U) [0:7] > + * - 20 > + - - > + - Hue Area 3 Lower Boundary (3L) [0:7] > + - - > + - Hue Area 3 Upper Boundary (3U) [0:7] > + * - 24 > + - - > + - Hue Area 4 Lower Boundary (4L) [0:7] > + - - > + - Hue Area 4 Upper Boundary (4U) [0:7] > + * - 28 > + - - > + - Hue Area 5 Lower Boundary (5L) [0:7] > + - - > + - Hue Area 5 Upper Boundary (5U) [0:7] What's the rationale for including the boundaries in the statistics buffer ? Boundaries are configured by userspace, they should be known to the application already. > + * - 32 > + - :cspan:`4` Histogram bucket (m=0, n=0) [31:0] > + * - 36 > + - :cspan:`4` Histogram bucket (m=0, n=1) [31:0] > + * - > + - :cspan:`4` ... > + * - 156 > + - :cspan:`4` Histogram bucket (m=0, n=31) [31:0] > + * - 160 > + - :cspan:`4` Histogram bucket (m=1, n=0) [31:0] > + * - > + - :cspan:`4` ... > + * - 288 > + - :cspan:`4` Histogram bucket (m=2, n=0) [31:0] > + * - > + - :cspan:`4` ... > + * - 416 > + - :cspan:`4` Histogram bucket (m=3, n=0) [31:0] > + * - > + - :cspan:`4` ... > + * - 544 > + - :cspan:`4` Histogram bucket (m=4, n=0) [31:0] > + * - > + - :cspan:`4` ... > + * - 672 > + - :cspan:`4` Histogram bucket (m=5, n=0) [31:0] > + * - > + - :cspan:`4` ... > + * - 796 > + - :cspan:`4` Histogram bucket (m=5, n=31) [31:0] [snip] -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine 2016-09-05 13:05 ` Laurent Pinchart @ 2016-09-05 13:34 ` Niklas Söderlund 0 siblings, 0 replies; 9+ messages in thread From: Niklas Söderlund @ 2016-09-05 13:34 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-renesas-soc, linux-media, corbet, mchehab, sakari.ailus, hans.verkuil Hi Laurent, Thanks for your review. On 2016-09-05 16:05:10 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Friday 02 Sep 2016 15:47:13 Niklas Söderlund wrote: > > The format is used on the R-Car VSP1 video queues that carry > > 2-D histogram statistics data. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > Documentation/media/uapi/v4l/meta-formats.rst | 1 + > > .../media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst | 150 ++++++++++++++++++ > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > > include/uapi/linux/videodev2.h | 3 +- > > 4 files changed, 154 insertions(+), 1 deletion(-) > > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst > > [snip] > > > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst > > b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst new file mode > > 100644 > > index 0000000..a093f0a > > --- /dev/null > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst > > @@ -0,0 +1,150 @@ > > +.. -*- coding: utf-8; mode: rst -*- > > + > > +.. _v4l2-meta-fmt-vsp1-hgt: > > + > > +******************************* > > +V4L2_META_FMT_VSP1_HGT ('VSPT') > > +******************************* > > + > > +*man V4L2_META_FMT_VSP1_HGT(2)* > > + > > +Renesas R-Car VSP1 2-D Histogram Data > > + > > + > > +Description > > +=========== > > + > > +This format describes histogram data generated by the Renesas R-Car VSP1 > > +2-D Histogram (HGT) engine. > > + > > +The VSP1 HGT is a histogram computation engine that operates on HSV > > +data. It operates on a possibly cropped and subsampled input image and > > +computes the sum, maximum and minimum of the S component as well as a > > +weighted frequency histogram based on the H and S components. > > + > > +The histogram is a matrix of 6 Hue and 32 Saturation buckets, 192 in > > +total. Each HSV value is added to one or more buckets with a wight > > s/wight/weight/ Will fix. > > > +between 1 and 16 depending on how the Hue areas are setup. > > I would say 'depending on the Hue areas configuration' to insist on the fact > that the configuration can be changed by the application. Will fix. > > > Finding the > > +correct buckets is done by inspecting the H and S value independently. > > Maybe s/correct/corresponding/ ? Will fix. > > > +The Saturation position **n** (0 - 31) in the matrix are found by the > > Maybe s/in the matrix/of the bucket/, or s/in the matrix/of the bucket in the > matrix/ ? > s/are found/is found/ or maybe 'is computed' ? Will fix. > > > +expression: > > + > > + 8 * n <= S < 8 * (n + 1) > > How about simply 'n = S / 8' ? Will fix. > > > +The Hue positions **m** (0 - 5) in the matrix depends on how the HGT Hue > > s/positions/position/ Will fix. > > > +areas are configured. There are 6 user configurable Hue Areas which can > > +be configured to cover overlapping Hue values: > > + > > +:: > > + > > + Area 0 Area 1 Area 2 Area 3 Area 4 > > Area 5 + ________ ________ ________ ________ > > ________ ________ + \ /| |\ /| |\ /| |\ /| > > |\ /| |\ /| |\ / + \ / | | \ / | | \ / | > > | \ / | | \ / | | \ / | | \ / + X | | X | | > > X | | X | | X | | X | | X + / \ | | / > > \ | | / \ | | / \ | | / \ | | / \ | | / \ + / > > \| |/ \| |/ \| |/ \| |/ \| |/ \| |/ > > \ + 5U 0L 0U 1L 1U 2L 2U 3L 3U 4L 4U > > 5L 5U 0L + <0..............................Hue > > Value............................255> > > + > > +As shown in the diagram a single Hue vale can be attributed to more then > > s/vale/value/ > s/then/than/ Will fix. > > > +one Hue area. In such case the Hue value is attributed to both Hue > > +Areas, but with a weight. The maximum weight is 16 and is associated > > +with all Hue values that are inside the center of a Hue area (between nL > > +-- nU). Values outside this area are weighted with a rounded down value > > +along the diagonal line. If there is no overlapped areas specified the > > +value is included in the lower area. > > This sounds a bit confusing to me. How about the following ? > > "The Hue position **m** (0 - 5) of the bucket [in the matrix]* depends on how > the HGT Hue areas are configured. There are 6 user configurable Hue Areas > which can be configured to cover overlapping Hue values: > > [diagram] > > When two consecutive areas don't overlap (n+1L is equal to nU) the boundary > value is considered as part of the lower area. > > Pixels with a hue value included in the centre of an area (between nL and nU > included) are are attributed to that single area and given a weight of 16. > Pixels with a hue value included in the overlapping region between two areas > (between n+1L and nU excluded) are attributed to both areas and given a weight > for each of these areas proportional to their position along the diagonal > lines (rounded down)." > > * Add "in the matrix" depending on the wording of the saturation description. > Will fix. > > +The Hue area setup must match one of the following constrains: > > + > > +:: > > + > > + 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U > > + > > +:: > > + > > + 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L > > + > > +**Byte Order.** > > +All data is stored in memory in little endian format. Each cell in the > > tables +contains one byte. > > + > > +.. flat-table:: VSP1 HGT Data - (800 bytes) > > + :header-rows: 2 > > + :stub-columns: 0 > > + > > + * - Offset > > + - :cspan:`4` Memory > > + * - > > + - [31:24] > > + - [23:16] > > + - [15:8] > > + - [7:0] > > + * - 0 > > + - - > > + - S max [7:0] > > + - - > > + - S min [7:0] > > + * - 4 > > + - :cspan:`4` S sum [31:0] > > + * - 8 > > + - - > > + - Hue Area 0 Lower Boundary (0L) [0:7] > > + - - > > + - Hue Area 0 Upper Boundary (0U) [0:7] > > + * - 12 > > + - - > > + - Hue Area 1 Lower Boundary (1L) [0:7] > > + - - > > + - Hue Area 1 Upper Boundary (1U) [0:7] > > + * - 16 > > + - - > > + - Hue Area 2 Lower Boundary (2L) [0:7] > > + - - > > + - Hue Area 2 Upper Boundary (2U) [0:7] > > + * - 20 > > + - - > > + - Hue Area 3 Lower Boundary (3L) [0:7] > > + - - > > + - Hue Area 3 Upper Boundary (3U) [0:7] > > + * - 24 > > + - - > > + - Hue Area 4 Lower Boundary (4L) [0:7] > > + - - > > + - Hue Area 4 Upper Boundary (4U) [0:7] > > + * - 28 > > + - - > > + - Hue Area 5 Lower Boundary (5L) [0:7] > > + - - > > + - Hue Area 5 Upper Boundary (5U) [0:7] > > What's the rationale for including the boundaries in the statistics buffer ? > Boundaries are configured by userspace, they should be known to the > application already. At the time I thought it would easy userspaces consumption of the data, for example if the histograms where recorded for later processing. Other then that I have no good rationale for including them. I be happy to drop them in v2 if you see no value in them. > > > + * - 32 > > + - :cspan:`4` Histogram bucket (m=0, n=0) [31:0] > > + * - 36 > > + - :cspan:`4` Histogram bucket (m=0, n=1) [31:0] > > + * - > > + - :cspan:`4` ... > > + * - 156 > > + - :cspan:`4` Histogram bucket (m=0, n=31) [31:0] > > + * - 160 > > + - :cspan:`4` Histogram bucket (m=1, n=0) [31:0] > > + * - > > + - :cspan:`4` ... > > + * - 288 > > + - :cspan:`4` Histogram bucket (m=2, n=0) [31:0] > > + * - > > + - :cspan:`4` ... > > + * - 416 > > + - :cspan:`4` Histogram bucket (m=3, n=0) [31:0] > > + * - > > + - :cspan:`4` ... > > + * - 544 > > + - :cspan:`4` Histogram bucket (m=4, n=0) [31:0] > > + * - > > + - :cspan:`4` ... > > + * - 672 > > + - :cspan:`4` Histogram bucket (m=5, n=0) [31:0] > > + * - > > + - :cspan:`4` ... > > + * - 796 > > + - :cspan:`4` Histogram bucket (m=5, n=31) [31:0] > > [snip] > > -- > Regards, > > Laurent Pinchart > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] v4l: vsp1: Add HGT support 2016-09-02 13:47 [PATCH 0/2] v4l: vsp1: Add HGT support Niklas Söderlund 2016-09-02 13:47 ` [PATCH 1/2] v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine Niklas Söderlund @ 2016-09-02 13:47 ` Niklas Söderlund 2016-09-05 15:43 ` Laurent Pinchart 1 sibling, 1 reply; 9+ messages in thread From: Niklas Söderlund @ 2016-09-02 13:47 UTC (permalink / raw) To: linux-renesas-soc, linux-media, laurent.pinchart Cc: corbet, mchehab, sakari.ailus, hans.verkuil, Niklas Söderlund The HGT is a Histogram Generator Two-Dimensions. It computes a weighted frequency histograms for hue and saturation areas over a configurable region of the image with optional subsampling. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/vsp1/Makefile | 2 +- drivers/media/platform/vsp1/vsp1.h | 3 + drivers/media/platform/vsp1/vsp1_drv.c | 32 +- drivers/media/platform/vsp1/vsp1_entity.c | 33 +- drivers/media/platform/vsp1/vsp1_entity.h | 1 + drivers/media/platform/vsp1/vsp1_hgt.c | 495 ++++++++++++++++++++++++++++++ drivers/media/platform/vsp1/vsp1_hgt.h | 51 +++ drivers/media/platform/vsp1/vsp1_pipe.c | 16 + drivers/media/platform/vsp1/vsp1_pipe.h | 2 + drivers/media/platform/vsp1/vsp1_regs.h | 9 + drivers/media/platform/vsp1/vsp1_video.c | 10 +- 11 files changed, 638 insertions(+), 16 deletions(-) create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h diff --git a/drivers/media/platform/vsp1/Makefile b/drivers/media/platform/vsp1/Makefile index 8ab6a06..a33afc3 100644 --- a/drivers/media/platform/vsp1/Makefile +++ b/drivers/media/platform/vsp1/Makefile @@ -3,7 +3,7 @@ vsp1-y += vsp1_dl.o vsp1_drm.o vsp1_video.o vsp1-y += vsp1_rpf.o vsp1_rwpf.o vsp1_wpf.o vsp1-y += vsp1_clu.o vsp1_hsit.o vsp1_lut.o vsp1-y += vsp1_bru.o vsp1_sru.o vsp1_uds.o -vsp1-y += vsp1_hgo.o vsp1_histo.o +vsp1-y += vsp1_hgo.o vsp1_hgt.o vsp1_histo.o vsp1-y += vsp1_lif.o obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1.o diff --git a/drivers/media/platform/vsp1/vsp1.h b/drivers/media/platform/vsp1/vsp1.h index 9dce3ea..012ce40 100644 --- a/drivers/media/platform/vsp1/vsp1.h +++ b/drivers/media/platform/vsp1/vsp1.h @@ -33,6 +33,7 @@ struct vsp1_platform_data; struct vsp1_bru; struct vsp1_clu; struct vsp1_hgo; +struct vsp1_hgt; struct vsp1_hsit; struct vsp1_lif; struct vsp1_lut; @@ -52,6 +53,7 @@ struct vsp1_uds; #define VSP1_HAS_WPF_VFLIP (1 << 5) #define VSP1_HAS_WPF_HFLIP (1 << 6) #define VSP1_HAS_HGO (1 << 7) +#define VSP1_HAS_HGT (1 << 8) struct vsp1_device_info { u32 version; @@ -74,6 +76,7 @@ struct vsp1_device { struct vsp1_bru *bru; struct vsp1_clu *clu; struct vsp1_hgo *hgo; + struct vsp1_hgt *hgt; struct vsp1_hsit *hsi; struct vsp1_hsit *hst; struct vsp1_lif *lif; diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c index 9684abf..828584f 100644 --- a/drivers/media/platform/vsp1/vsp1_drv.c +++ b/drivers/media/platform/vsp1/vsp1_drv.c @@ -31,6 +31,7 @@ #include "vsp1_dl.h" #include "vsp1_drm.h" #include "vsp1_hgo.h" +#include "vsp1_hgt.h" #include "vsp1_hsit.h" #include "vsp1_lif.h" #include "vsp1_lut.h" @@ -107,6 +108,7 @@ static int vsp1_create_sink_links(struct vsp1_device *vsp1, continue; if (source->type == VSP1_ENTITY_HGO || + source->type == VSP1_ENTITY_HGT || source->type == VSP1_ENTITY_LIF || source->type == VSP1_ENTITY_WPF) continue; @@ -160,6 +162,16 @@ static int vsp1_uapi_create_links(struct vsp1_device *vsp1) return ret; } + if (vsp1->hgt) { + ret = media_create_pad_link(&vsp1->hgt->entity.subdev.entity, + HGT_PAD_SOURCE, + &vsp1->hgt->histo.video.entity, 0, + MEDIA_LNK_FL_ENABLED | + MEDIA_LNK_FL_IMMUTABLE); + if (ret < 0) + return ret; + } + if (vsp1->lif) { ret = media_create_pad_link(&vsp1->wpf[0]->entity.subdev.entity, RWPF_PAD_SOURCE, @@ -300,6 +312,16 @@ static int vsp1_create_entities(struct vsp1_device *vsp1) list_add_tail(&vsp1->hgo->entity.list_dev, &vsp1->entities); } + if (vsp1->info->features & VSP1_HAS_HGT && vsp1->info->uapi) { + vsp1->hgt = vsp1_hgt_create(vsp1); + if (IS_ERR(vsp1->hgt)) { + ret = PTR_ERR(vsp1->hgt); + goto done; + } + + list_add_tail(&vsp1->hgt->entity.list_dev, &vsp1->entities); + } + /* The LIF is only supported when used in conjunction with the DU, in * which case the userspace API is disabled. If the userspace API is * enabled skip the LIF, even when present. @@ -583,7 +605,8 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .version = VI6_IP_VERSION_MODEL_VSPS_H2, .gen = 2, .features = VSP1_HAS_BRU | VSP1_HAS_CLU | VSP1_HAS_HGO - | VSP1_HAS_LUT | VSP1_HAS_SRU | VSP1_HAS_WPF_VFLIP, + | VSP1_HAS_HGT | VSP1_HAS_LUT | VSP1_HAS_SRU + | VSP1_HAS_WPF_VFLIP, .rpf_count = 5, .uds_count = 3, .wpf_count = 4, @@ -612,7 +635,8 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .version = VI6_IP_VERSION_MODEL_VSPS_M2, .gen = 2, .features = VSP1_HAS_BRU | VSP1_HAS_CLU | VSP1_HAS_HGO - | VSP1_HAS_LUT | VSP1_HAS_SRU | VSP1_HAS_WPF_VFLIP, + | VSP1_HAS_HGT | VSP1_HAS_LUT | VSP1_HAS_SRU + | VSP1_HAS_WPF_VFLIP, .rpf_count = 5, .uds_count = 1, .wpf_count = 4, @@ -621,8 +645,8 @@ static const struct vsp1_device_info vsp1_device_infos[] = { }, { .version = VI6_IP_VERSION_MODEL_VSPI_GEN3, .gen = 3, - .features = VSP1_HAS_CLU | VSP1_HAS_HGO | VSP1_HAS_LUT - | VSP1_HAS_SRU | VSP1_HAS_WPF_HFLIP + .features = VSP1_HAS_CLU | VSP1_HAS_HGO | VSP1_HAS_HGT + | VSP1_HAS_LUT | VSP1_HAS_SRU | VSP1_HAS_WPF_HFLIP | VSP1_HAS_WPF_VFLIP, .rpf_count = 1, .uds_count = 1, diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c index a6b7d87..0a0c46e 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.c +++ b/drivers/media/platform/vsp1/vsp1_entity.c @@ -49,6 +49,18 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity, vsp1_dl_list_write(dl, VI6_DPR_HGO_SMPPT, smppt); return; + } else if (entity->type == VSP1_ENTITY_HGT) { + u32 smppt; + + /* The HGT is a special case, its routing is configured on the + * sink pad. + */ + source = media_entity_to_vsp1_entity(entity->sources[0]); + smppt = (pipe->output->entity.index << VI6_DPR_SMPPT_TGW_SHIFT) + | (source->route->output << VI6_DPR_SMPPT_PT_SHIFT); + + vsp1_dl_list_write(dl, VI6_DPR_HGT_SMPPT, smppt); + return; } source = entity; @@ -302,9 +314,10 @@ static int vsp1_entity_link_setup_source(const struct media_pad *source_pad, = media_entity_to_vsp1_entity(sink_pad->entity); /* Fan-out is limited to one for the normal data path plus an - * optional HGO. We ignore the HGO here. + * optional HGO or HGT. We ignore the HGO and HGT here. */ - if (sink->type != VSP1_ENTITY_HGO) { + if (sink->type != VSP1_ENTITY_HGO && + sink->type != VSP1_ENTITY_HGT) { if (source->sink) return -EBUSY; source->sink = sink_pad->entity; @@ -357,10 +370,10 @@ int vsp1_entity_link_setup(struct media_entity *entity, * links originating or terminating at that pad until an enabled link is found. * * Our link setup implementation guarantees that the output fan-out will not be - * higher than one for the data pipelines, except for the link to the HGO that - * can be enabled in addition to a regular data link. When traversing outgoing - * links this function ignores HGO entities and should thus be used in place of - * the generic media_entity_remote_pad() function when traversing data + * higher than one for the data pipelines, except for the link to the HGO or HGT + * that can be enabled in addition to a regular data link. When traversing + * outgoing links this function ignores HGO entities and should thus be used in + * place of the generic media_entity_remote_pad() function when traversing data * pipelines. * * Return a pointer to the pad at the remote end of the first found enabled @@ -376,19 +389,20 @@ struct media_pad *vsp1_entity_remote_pad(struct media_pad *pad) if (!(link->flags & MEDIA_LNK_FL_ENABLED)) continue; - /* If we're the sink the source will never be an HGO. */ + /* If we're the sink the source will never be an HGO or HGT. */ if (link->sink == pad) return link->source; if (link->source != pad) continue; - /* If the sink isn't a subdevice it can't be an HGO. */ + /* If the sink isn't a subdevice it can't be an HGO or HGT. */ if (!is_media_entity_v4l2_subdev(link->sink->entity)) return link->sink; entity = media_entity_to_vsp1_entity(link->sink->entity); - if (entity->type != VSP1_ENTITY_HGO) + if (entity->type != VSP1_ENTITY_HGO && + entity->type != VSP1_ENTITY_HGT) return link->sink; } @@ -423,6 +437,7 @@ static const struct vsp1_route vsp1_routes[] = { VI6_DPR_NODE_BRU_IN(4) }, VI6_DPR_NODE_BRU_OUT }, VSP1_ENTITY_ROUTE(CLU), { VSP1_ENTITY_HGO, 0, 0, { 0, }, 0 }, + { VSP1_ENTITY_HGT, 0, 0, { 0, }, 0 }, VSP1_ENTITY_ROUTE(HSI), VSP1_ENTITY_ROUTE(HST), { VSP1_ENTITY_LIF, 0, 0, { VI6_DPR_NODE_LIF, }, VI6_DPR_NODE_LIF }, diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h index 4fc2cd1..0682400 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.h +++ b/drivers/media/platform/vsp1/vsp1_entity.h @@ -26,6 +26,7 @@ enum vsp1_entity_type { VSP1_ENTITY_BRU, VSP1_ENTITY_CLU, VSP1_ENTITY_HGO, + VSP1_ENTITY_HGT, VSP1_ENTITY_HSI, VSP1_ENTITY_HST, VSP1_ENTITY_LIF, diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c b/drivers/media/platform/vsp1/vsp1_hgt.c new file mode 100644 index 0000000..c43373d --- /dev/null +++ b/drivers/media/platform/vsp1/vsp1_hgt.c @@ -0,0 +1,495 @@ +/* + * vsp1_hgt.c -- R-Car VSP1 Histogram Generator 2D + * + * Copyright (C) 2016 Renesas Electronics Corporation + * + * Contact: Niklas Söderlund (niklas.soderlund@ragnatech.se) + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/device.h> +#include <linux/gfp.h> + +#include <media/v4l2-subdev.h> +#include <media/videobuf2-vmalloc.h> + +#include "vsp1.h" +#include "vsp1_dl.h" +#include "vsp1_hgt.h" + +#define HGT_MIN_SIZE 4U +#define HGT_MAX_SIZE 8192U +#define HGT_DATA_SIZE ((2 + 6 + 6 * 32) * 4) + +/* ----------------------------------------------------------------------------- + * Device Access + */ + +static inline u32 vsp1_hgt_read(struct vsp1_hgt *hgt, u32 reg) +{ + return vsp1_read(hgt->entity.vsp1, reg); +} + +static inline void vsp1_hgt_write(struct vsp1_hgt *hgt, struct vsp1_dl_list *dl, + u32 reg, u32 data) +{ + vsp1_dl_list_write(dl, reg, data); +} + +/* ----------------------------------------------------------------------------- + * Frame End Handler + */ + +void vsp1_hgt_frame_end(struct vsp1_entity *entity) +{ + struct vsp1_hgt *hgt = to_hgt(&entity->subdev); + struct vsp1_histogram_buffer *buf; + unsigned int m, n; + u32 *data; + + buf = vsp1_histogram_buffer_get(&hgt->histo); + if (!buf) + return; + + data = buf->addr; + + *data++ = vsp1_hgt_read(hgt, VI6_HGT_MAXMIN); + *data++ = vsp1_hgt_read(hgt, VI6_HGT_SUM); + + for (n = 0; n < 6; n++) + *data++ = vsp1_hgt_read(hgt, VI6_HGT_HUE_AREA(n)); + + for (m = 0; m < 6; m++) + for (n = 0; n < 32; n++) + *data++ = vsp1_hgt_read(hgt, VI6_HGT_HISTO(m, n)); + + vsp1_histogram_buffer_complete(&hgt->histo, buf, HGT_DATA_SIZE); +} + +/* ----------------------------------------------------------------------------- + * Controls + */ + +#define V4L2_CID_VSP1_HGT_HUE_AREAS (V4L2_CID_USER_BASE | 0x1001) + +static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl) +{ + struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt, + ctrls.handler); + int m, n; + bool ok; + + /* + * Make sure values meet one of two possible HW constrains + * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U + * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L + */ + for (m = 0; m <= 1; m++) { + ok = true; + for (n = 0; n < HGT_NUM_HUE_AREAS - 1; n++) { + if (ctrl->p_new.p_u8[(m + n + 0) % HGT_NUM_HUE_AREAS] > + ctrl->p_new.p_u8[(m + n + 1) % HGT_NUM_HUE_AREAS]) + ok = false; + } + if (ok) + break; + } + + /* Values do not match HW, adjust to a valid setting */ + if (!ok) { + for (n = 0; n < HGT_NUM_HUE_AREAS - 1; n++) { + if (ctrl->p_new.p_u8[n] > ctrl->p_new.p_u8[n+1]) + ctrl->p_new.p_u8[n] = ctrl->p_new.p_u8[n+1]; + } + } + + for (n = 0; n < HGT_NUM_HUE_AREAS; n++) + hgt->hue_area[n] = ctrl->p_new.p_u8[n]; + + return 0; +} + +static const struct v4l2_ctrl_ops hgt_hue_areas_ctrl_ops = { + .s_ctrl = hgt_hue_areas_s_ctrl, +}; + +static const struct v4l2_ctrl_config hgt_hue_areas = { + .ops = &hgt_hue_areas_ctrl_ops, + .id = V4L2_CID_VSP1_HGT_HUE_AREAS, + .name = "Boundary Values for Hue Area", + .type = V4L2_CTRL_TYPE_U8, + .min = 0, + .max = 255, + .def = 0, + .step = 1, + .dims = { 12 }, +}; + + +/* ----------------------------------------------------------------------------- + * V4L2 Subdevice Operations + */ + +static int hgt_enum_mbus_code(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_mbus_code_enum *code) +{ + static const unsigned int codes[] = { + MEDIA_BUS_FMT_AHSV8888_1X32, + }; + + if (code->pad == HGT_PAD_SOURCE) { + code->code = MEDIA_BUS_FMT_FIXED; + return 0; + } + + return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes, + ARRAY_SIZE(codes)); +} + +static int hgt_enum_frame_size(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_frame_size_enum *fse) +{ + if (fse->pad != HGT_PAD_SINK) + return -EINVAL; + + return vsp1_subdev_enum_frame_size(subdev, cfg, fse, HGT_MIN_SIZE, + HGT_MIN_SIZE, HGT_MAX_SIZE, + HGT_MAX_SIZE); +} + +static int hgt_get_selection(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_selection *sel) +{ + struct vsp1_hgt *hgt = to_hgt(subdev); + struct v4l2_subdev_pad_config *config; + struct v4l2_mbus_framefmt *format; + struct v4l2_rect *crop; + + if (sel->pad != HGT_PAD_SINK) + return -EINVAL; + + config = vsp1_entity_get_pad_config(&hgt->entity, cfg, sel->which); + if (!config) + return -EINVAL; + + switch (sel->target) { + case V4L2_SEL_TGT_COMPOSE_BOUNDS: + case V4L2_SEL_TGT_COMPOSE_DEFAULT: + crop = vsp1_entity_get_pad_selection(&hgt->entity, config, + HGT_PAD_SINK, + V4L2_SEL_TGT_CROP); + sel->r.left = 0; + sel->r.top = 0; + sel->r.width = crop->width; + sel->r.height = crop->height; + return 0; + + case V4L2_SEL_TGT_CROP_BOUNDS: + case V4L2_SEL_TGT_CROP_DEFAULT: + format = vsp1_entity_get_pad_format(&hgt->entity, config, + HGT_PAD_SINK); + sel->r.left = 0; + sel->r.top = 0; + sel->r.width = format->width; + sel->r.height = format->height; + return 0; + + case V4L2_SEL_TGT_COMPOSE: + case V4L2_SEL_TGT_CROP: + sel->r = *vsp1_entity_get_pad_selection(&hgt->entity, config, + sel->pad, sel->target); + return 0; + + default: + return -EINVAL; + } +} + +static int hgt_set_crop(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *config, + struct v4l2_subdev_selection *sel) +{ + struct vsp1_hgt *hgt = to_hgt(subdev); + struct v4l2_mbus_framefmt *format; + struct v4l2_rect *selection; + + /* The crop rectangle must be inside the input frame. */ + format = vsp1_entity_get_pad_format(&hgt->entity, config, HGT_PAD_SINK); + sel->r.left = clamp_t(unsigned int, sel->r.left, 0, format->width - 1); + sel->r.top = clamp_t(unsigned int, sel->r.top, 0, format->height - 1); + sel->r.width = clamp_t(unsigned int, sel->r.width, HGT_MIN_SIZE, + format->width - sel->r.left); + sel->r.height = clamp_t(unsigned int, sel->r.height, HGT_MIN_SIZE, + format->height - sel->r.top); + + /* Set the crop rectangle and reset the compose rectangle. */ + selection = vsp1_entity_get_pad_selection(&hgt->entity, config, + sel->pad, V4L2_SEL_TGT_CROP); + *selection = sel->r; + + selection = vsp1_entity_get_pad_selection(&hgt->entity, config, + sel->pad, + V4L2_SEL_TGT_COMPOSE); + *selection = sel->r; + + return 0; +} + +static int hgt_set_compose(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *config, + struct v4l2_subdev_selection *sel) +{ + struct vsp1_hgt *hgt = to_hgt(subdev); + struct v4l2_rect *compose; + struct v4l2_rect *crop; + unsigned int ratio; + + /* The compose rectangle is used to configure downscaling, the top left + * corner is fixed to (0,0) and the size to 1/2 or 1/4 of the crop + * rectangle. + */ + sel->r.left = 0; + sel->r.top = 0; + + crop = vsp1_entity_get_pad_selection(&hgt->entity, config, sel->pad, + V4L2_SEL_TGT_CROP); + + /* Clamp the width and height to acceptable values first and then + * compute the closest rounded dividing ratio. + * + * Ratio Rounded ratio + * -------------------------- + * [1.0 1.5[ 1 + * [1.5 3.0[ 2 + * [3.0 4.0] 4 + * + * The rounded ratio can be computed using + * + * 1 << (ceil(ratio * 2) / 3) + */ + sel->r.width = clamp(sel->r.width, crop->width / 4, crop->width); + ratio = 1 << (crop->width * 2 / sel->r.width / 3); + sel->r.width = crop->width / ratio; + + + sel->r.height = clamp(sel->r.height, crop->height / 4, crop->height); + ratio = 1 << (crop->height * 2 / sel->r.height / 3); + sel->r.height = crop->height / ratio; + + compose = vsp1_entity_get_pad_selection(&hgt->entity, config, sel->pad, + V4L2_SEL_TGT_COMPOSE); + *compose = sel->r; + + return 0; +} + +static int hgt_set_selection(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_selection *sel) +{ + struct vsp1_hgt *hgt = to_hgt(subdev); + struct v4l2_subdev_pad_config *config; + + if (sel->pad != HGT_PAD_SINK) + return -EINVAL; + + config = vsp1_entity_get_pad_config(&hgt->entity, cfg, sel->which); + if (!config) + return -EINVAL; + + if (sel->target == V4L2_SEL_TGT_CROP) + return hgt_set_crop(subdev, config, sel); + else if (sel->target == V4L2_SEL_TGT_COMPOSE) + return hgt_set_compose(subdev, config, sel); + else + return -EINVAL; +} + +static int hgt_get_format(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *fmt) +{ + if (fmt->pad == HGT_PAD_SOURCE) { + fmt->format.code = MEDIA_BUS_FMT_FIXED; + fmt->format.width = 0; + fmt->format.height = 0; + fmt->format.field = V4L2_FIELD_NONE; + fmt->format.colorspace = V4L2_COLORSPACE_RAW; + return 0; + } + + return vsp1_subdev_get_pad_format(subdev, cfg, fmt); +} + +static int hgt_set_format(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *fmt) +{ + struct vsp1_hgt *hgt = to_hgt(subdev); + struct v4l2_subdev_pad_config *config; + struct v4l2_mbus_framefmt *format; + struct v4l2_rect *selection; + + if (fmt->pad != HGT_PAD_SINK) + return hgt_get_format(subdev, cfg, fmt); + + config = vsp1_entity_get_pad_config(&hgt->entity, cfg, fmt->which); + if (!config) + return -EINVAL; + + /* HSV is the only supported format */ + fmt->format.code = MEDIA_BUS_FMT_AHSV8888_1X32; + + format = vsp1_entity_get_pad_format(&hgt->entity, config, fmt->pad); + + format->code = fmt->format.code; + format->width = clamp_t(unsigned int, fmt->format.width, + HGT_MIN_SIZE, HGT_MAX_SIZE); + format->height = clamp_t(unsigned int, fmt->format.height, + HGT_MIN_SIZE, HGT_MAX_SIZE); + format->field = V4L2_FIELD_NONE; + format->colorspace = V4L2_COLORSPACE_SRGB; + + fmt->format = *format; + + /* Reset the crop and compose rectangles */ + selection = vsp1_entity_get_pad_selection(&hgt->entity, config, + fmt->pad, V4L2_SEL_TGT_CROP); + selection->left = 0; + selection->top = 0; + selection->width = format->width; + selection->height = format->height; + + selection = vsp1_entity_get_pad_selection(&hgt->entity, config, + fmt->pad, + V4L2_SEL_TGT_COMPOSE); + selection->left = 0; + selection->top = 0; + selection->width = format->width; + selection->height = format->height; + + return 0; +} + +static const struct v4l2_subdev_pad_ops hgt_pad_ops = { + .enum_mbus_code = hgt_enum_mbus_code, + .enum_frame_size = hgt_enum_frame_size, + .get_fmt = hgt_get_format, + .set_fmt = hgt_set_format, + .get_selection = hgt_get_selection, + .set_selection = hgt_set_selection, +}; + +static const struct v4l2_subdev_ops hgt_ops = { + .pad = &hgt_pad_ops, +}; + +/* ----------------------------------------------------------------------------- + * VSP1 Entity Operations + */ + +static void hgt_configure(struct vsp1_entity *entity, + struct vsp1_pipeline *pipe, + struct vsp1_dl_list *dl, bool full) +{ + struct vsp1_hgt *hgt = to_hgt(&entity->subdev); + struct v4l2_rect *compose; + struct v4l2_rect *crop; + unsigned int hratio; + unsigned int vratio; + uint8_t lower, upper; + int i; + + if (!full) + return; + + crop = vsp1_entity_get_pad_selection(entity, entity->config, + HGT_PAD_SINK, V4L2_SEL_TGT_CROP); + compose = vsp1_entity_get_pad_selection(entity, entity->config, + HGT_PAD_SINK, + V4L2_SEL_TGT_COMPOSE); + + vsp1_hgt_write(hgt, dl, VI6_HGT_REGRST, VI6_HGT_REGRST_RCLEA); + + vsp1_hgt_write(hgt, dl, VI6_HGT_OFFSET, + (crop->left << VI6_HGT_OFFSET_HOFFSET_SHIFT) | + (crop->top << VI6_HGT_OFFSET_VOFFSET_SHIFT)); + vsp1_hgt_write(hgt, dl, VI6_HGT_SIZE, + (crop->width << VI6_HGT_SIZE_HSIZE_SHIFT) | + (crop->height << VI6_HGT_SIZE_VSIZE_SHIFT)); + + mutex_lock(hgt->ctrls.handler.lock); + for (i = 0; i < 6; i++) { + lower = hgt->hue_area[i*2 + 0]; + upper = hgt->hue_area[i*2 + 1]; + vsp1_hgt_write(hgt, dl, VI6_HGT_HUE_AREA(i), + (lower << VI6_HGT_HUE_AREA_LOWER_SHIFT) | + (upper << VI6_HGT_HUE_AREA_UPPER_SHIFT)); + } + mutex_unlock(hgt->ctrls.handler.lock); + + hratio = crop->width * 2 / compose->width / 3; + vratio = crop->height * 2 / compose->height / 3; + vsp1_hgt_write(hgt, dl, VI6_HGT_MODE, + (hratio << VI6_HGT_MODE_HRATIO_SHIFT) | + (vratio << VI6_HGT_MODE_VRATIO_SHIFT)); +} + +static void hgt_destroy(struct vsp1_entity *entity) +{ + struct vsp1_hgt *hgt = to_hgt(&entity->subdev); + + vsp1_histogram_cleanup(&hgt->histo); +} + +static const struct vsp1_entity_operations hgt_entity_ops = { + .configure = hgt_configure, + .destroy = hgt_destroy, +}; + +/* ----------------------------------------------------------------------------- + * Initialization and Cleanup + */ + +struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1) +{ + struct vsp1_hgt *hgt; + int i, ret; + + hgt = devm_kzalloc(vsp1->dev, sizeof(*hgt), GFP_KERNEL); + if (hgt == NULL) + return ERR_PTR(-ENOMEM); + + hgt->entity.ops = &hgt_entity_ops; + hgt->entity.type = VSP1_ENTITY_HGT; + + ret = vsp1_entity_init(vsp1, &hgt->entity, "hgt", 2, &hgt_ops, + MEDIA_ENT_F_PROC_VIDEO_STATISTICS); + if (ret < 0) + return ERR_PTR(ret); + + /* Initialize the control handler. */ + for (i = 0; i < HGT_NUM_HUE_AREAS; i++) + hgt->hue_area[i] = hgt_hue_areas.def; + v4l2_ctrl_handler_init(&hgt->ctrls.handler, 12); + hgt->ctrls.hue_areas = v4l2_ctrl_new_custom(&hgt->ctrls.handler, + &hgt_hue_areas, NULL); + hgt->entity.subdev.ctrl_handler = &hgt->ctrls.handler; + + /* Initialize the video device and queue for statistics data. */ + ret = vsp1_histogram_init(vsp1, &hgt->histo, hgt->entity.subdev.name, + HGT_DATA_SIZE, V4L2_META_FMT_VSP1_HGT); + if (ret < 0) { + vsp1_entity_destroy(&hgt->entity); + return ERR_PTR(ret); + } + + return hgt; +} diff --git a/drivers/media/platform/vsp1/vsp1_hgt.h b/drivers/media/platform/vsp1/vsp1_hgt.h new file mode 100644 index 0000000..a2f1eae --- /dev/null +++ b/drivers/media/platform/vsp1/vsp1_hgt.h @@ -0,0 +1,51 @@ +/* + * vsp1_hgt.h -- R-Car VSP1 Histogram Generator 2D + * + * Copyright (C) 2016 Renesas Electronics Corporation + * + * Contact: Niklas Söderlund (niklas.soderlund@ragnatech.se) + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ +#ifndef __VSP1_HGT_H__ +#define __VSP1_HGT_H__ + +#include <media/media-entity.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-subdev.h> + +#include "vsp1_entity.h" +#include "vsp1_histo.h" + +struct vsp1_device; + +#define HGT_PAD_SINK 0 +#define HGT_PAD_SOURCE 1 + +#define HGT_NUM_HUE_AREAS 12 + +struct vsp1_hgt { + struct vsp1_entity entity; + struct vsp1_histogram histo; + + struct { + struct v4l2_ctrl_handler handler; + struct v4l2_ctrl *hue_areas; + } ctrls; + + unsigned int hue_area[HGT_NUM_HUE_AREAS]; +}; + + +static inline struct vsp1_hgt *to_hgt(struct v4l2_subdev *subdev) +{ + return container_of(subdev, struct vsp1_hgt, entity.subdev); +} + +struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1); +void vsp1_hgt_frame_end(struct vsp1_entity *hgt); + +#endif /* __VSP1_HGT_H__ */ diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c index 0dd7c16..052a603 100644 --- a/drivers/media/platform/vsp1/vsp1_pipe.c +++ b/drivers/media/platform/vsp1/vsp1_pipe.c @@ -24,6 +24,7 @@ #include "vsp1_dl.h" #include "vsp1_entity.h" #include "vsp1_hgo.h" +#include "vsp1_hgt.h" #include "vsp1_pipe.h" #include "vsp1_rwpf.h" #include "vsp1_uds.h" @@ -191,12 +192,19 @@ void vsp1_pipeline_reset(struct vsp1_pipeline *pipe) hgo->histo.pipe = NULL; } + if (pipe->hgt) { + struct vsp1_hgt *hgt = to_hgt(&pipe->hgt->subdev); + + hgt->histo.pipe = NULL; + } + INIT_LIST_HEAD(&pipe->entities); pipe->state = VSP1_PIPELINE_STOPPED; pipe->buffers_ready = 0; pipe->num_inputs = 0; pipe->bru = NULL; pipe->hgo = NULL; + pipe->hgt = NULL; pipe->lif = NULL; pipe->uds = NULL; } @@ -278,6 +286,11 @@ int vsp1_pipeline_stop(struct vsp1_pipeline *pipe) (7 << VI6_DPR_SMPPT_TGW_SHIFT) | (VI6_DPR_NODE_UNUSED << VI6_DPR_SMPPT_PT_SHIFT)); + if (pipe->hgt) + vsp1_write(vsp1, VI6_DPR_HGT_SMPPT, + (7 << VI6_DPR_SMPPT_TGW_SHIFT) | + (VI6_DPR_NODE_UNUSED << VI6_DPR_SMPPT_PT_SHIFT)); + v4l2_subdev_call(&pipe->output->entity.subdev, video, s_stream, 0); return ret; @@ -304,6 +317,9 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe) if (pipe->hgo) vsp1_hgo_frame_end(pipe->hgo); + if (pipe->hgt) + vsp1_hgt_frame_end(pipe->hgt); + if (pipe->frame_end) pipe->frame_end(pipe); diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h index bd42eff..740bde2 100644 --- a/drivers/media/platform/vsp1/vsp1_pipe.h +++ b/drivers/media/platform/vsp1/vsp1_pipe.h @@ -73,6 +73,7 @@ enum vsp1_pipeline_state { * @output: WPF at the output of the pipeline * @bru: BRU entity, if present * @hgo: HGO entity, if present + * @hgt: HGT entity, if present * @lif: LIF entity, if present * @uds: UDS entity, if present * @uds_input: entity at the input of the UDS, if the UDS is present @@ -99,6 +100,7 @@ struct vsp1_pipeline { struct vsp1_rwpf *output; struct vsp1_entity *bru; struct vsp1_entity *hgo; + struct vsp1_entity *hgt; struct vsp1_entity *lif; struct vsp1_entity *uds; struct vsp1_entity *uds_input; diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h index d821348..d1d9af9 100644 --- a/drivers/media/platform/vsp1/vsp1_regs.h +++ b/drivers/media/platform/vsp1/vsp1_regs.h @@ -628,9 +628,17 @@ */ #define VI6_HGT_OFFSET 0x3400 +#define VI6_HGT_OFFSET_HOFFSET_SHIFT 16 +#define VI6_HGT_OFFSET_VOFFSET_SHIFT 0 #define VI6_HGT_SIZE 0x3404 +#define VI6_HGT_SIZE_HSIZE_SHIFT 16 +#define VI6_HGT_SIZE_VSIZE_SHIFT 0 #define VI6_HGT_MODE 0x3408 +#define VI6_HGT_MODE_HRATIO_SHIFT 2 +#define VI6_HGT_MODE_VRATIO_SHIFT 0 #define VI6_HGT_HUE_AREA(n) (0x340c + (n) * 4) +#define VI6_HGT_HUE_AREA_LOWER_SHIFT 16 +#define VI6_HGT_HUE_AREA_UPPER_SHIFT 0 #define VI6_HGT_LB_TH 0x3424 #define VI6_HGT_LBn_H(n) (0x3438 + (n) * 8) #define VI6_HGT_LBn_V(n) (0x342c + (n) * 8) @@ -639,6 +647,7 @@ #define VI6_HGT_SUM 0x3754 #define VI6_HGT_LB_DET 0x3758 #define VI6_HGT_REGRST 0x37fc +#define VI6_HGT_REGRST_RCLEA (1 << 0) /* ----------------------------------------------------------------------------- * LIF Control Registers diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c index 75e6e6c..7215e08 100644 --- a/drivers/media/platform/vsp1/vsp1_video.c +++ b/drivers/media/platform/vsp1/vsp1_video.c @@ -32,6 +32,7 @@ #include "vsp1_dl.h" #include "vsp1_entity.h" #include "vsp1_hgo.h" +#include "vsp1_hgt.h" #include "vsp1_pipe.h" #include "vsp1_rwpf.h" #include "vsp1_uds.h" @@ -326,7 +327,7 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe, if (ret < 0) return ret; - /* The main data path doesn't include the HGO, use + /* The main data path doesn't include the HGO or HGT, use * vsp1_entity_remote_pad() to traverse the graph. */ @@ -382,7 +383,7 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe, : &input->entity; } - /* Follow the source link, ignoring any HGO. */ + /* Follow the source link, ignoring any HGO or HGT. */ pad = &entity->pads[entity->source_pad]; pad = vsp1_entity_remote_pad(pad); } @@ -444,6 +445,11 @@ static int vsp1_video_pipeline_build(struct vsp1_pipeline *pipe, pipe->hgo = e; hgo->histo.pipe = pipe; + } else if (e->type == VSP1_ENTITY_HGT) { + struct vsp1_hgt *hgt = to_hgt(subdev); + + pipe->hgt = e; + hgt->histo.pipe = pipe; } } -- 2.9.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] v4l: vsp1: Add HGT support 2016-09-02 13:47 ` [PATCH 2/2] v4l: vsp1: Add HGT support Niklas Söderlund @ 2016-09-05 15:43 ` Laurent Pinchart 2016-09-05 15:57 ` Geert Uytterhoeven 2016-09-06 6:45 ` Niklas Söderlund 0 siblings, 2 replies; 9+ messages in thread From: Laurent Pinchart @ 2016-09-05 15:43 UTC (permalink / raw) To: Niklas Söderlund Cc: linux-renesas-soc, linux-media, corbet, mchehab, sakari.ailus, hans.verkuil Hi Niklas, Thank you for the patch. On Friday 02 Sep 2016 15:47:14 Niklas Söderlund wrote: > The HGT is a Histogram Generator Two-Dimensions. It computes a weighted > frequency histograms for hue and saturation areas over a configurable > region of the image with optional subsampling. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/vsp1/Makefile | 2 +- > drivers/media/platform/vsp1/vsp1.h | 3 + > drivers/media/platform/vsp1/vsp1_drv.c | 32 +- > drivers/media/platform/vsp1/vsp1_entity.c | 33 +- > drivers/media/platform/vsp1/vsp1_entity.h | 1 + > drivers/media/platform/vsp1/vsp1_hgt.c | 495 +++++++++++++++++++++++++++ > drivers/media/platform/vsp1/vsp1_hgt.h | 51 +++ > drivers/media/platform/vsp1/vsp1_pipe.c | 16 + > drivers/media/platform/vsp1/vsp1_pipe.h | 2 + > drivers/media/platform/vsp1/vsp1_regs.h | 9 + > drivers/media/platform/vsp1/vsp1_video.c | 10 +- > 11 files changed, 638 insertions(+), 16 deletions(-) > create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c > create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h [snip] > diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c > b/drivers/media/platform/vsp1/vsp1_hgt.c new file mode 100644 > index 0000000..c43373d > --- /dev/null > +++ b/drivers/media/platform/vsp1/vsp1_hgt.c > @@ -0,0 +1,495 @@ > +/* > + * vsp1_hgt.c -- R-Car VSP1 Histogram Generator 2D > + * > + * Copyright (C) 2016 Renesas Electronics Corporation > + * > + * Contact: Niklas Söderlund (niklas.soderlund@ragnatech.se) > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <linux/device.h> > +#include <linux/gfp.h> > + > +#include <media/v4l2-subdev.h> > +#include <media/videobuf2-vmalloc.h> > + > +#include "vsp1.h" > +#include "vsp1_dl.h" > +#include "vsp1_hgt.h" > + > +#define HGT_MIN_SIZE 4U > +#define HGT_MAX_SIZE 8192U > +#define HGT_DATA_SIZE ((2 + 6 + 6 * 32) * 4) > + > +/* ------------------------------------------------------------------------ > + * Device Access > + */ > + > +static inline u32 vsp1_hgt_read(struct vsp1_hgt *hgt, u32 reg) > +{ > + return vsp1_read(hgt->entity.vsp1, reg); > +} > + > +static inline void vsp1_hgt_write(struct vsp1_hgt *hgt, struct vsp1_dl_list > *dl, > + u32 reg, u32 data) > +{ > + vsp1_dl_list_write(dl, reg, data); > +} > + > +/* ------------------------------------------------------------------------ > + * Frame End Handler > + */ > + > +void vsp1_hgt_frame_end(struct vsp1_entity *entity) > +{ > + struct vsp1_hgt *hgt = to_hgt(&entity->subdev); > + struct vsp1_histogram_buffer *buf; > + unsigned int m, n; > + u32 *data; > + > + buf = vsp1_histogram_buffer_get(&hgt->histo); > + if (!buf) > + return; > + > + data = buf->addr; > + > + *data++ = vsp1_hgt_read(hgt, VI6_HGT_MAXMIN); > + *data++ = vsp1_hgt_read(hgt, VI6_HGT_SUM); > + > + for (n = 0; n < 6; n++) Nitpicking, the driver uses pre-increment in for loops (++n), not post- increment. This used to be a best-practice rule in C++, where pre-increment can be faster for non-native types (see http://antonym.org/2008/05/stl-iterators-and-performance.html for instance). I'm not sure if that's still relevant, but I've taken the habit of using the pre-increment operator in for loops, and that's what the rest of this driver does. This comment applies to all other locations in this file. > + *data++ = vsp1_hgt_read(hgt, VI6_HGT_HUE_AREA(n)); As commented on patch 1/2, I don't think this is needed. Userspace has configured the hue areas, it should already have access to this information. Note that if you wanted to keep this code you would need to synchronize it with the .s_ctrl() handler to avoid race conditions. That's not trivial, and in my opinion not needed. > + for (m = 0; m < 6; m++) > + for (n = 0; n < 32; n++) > + *data++ = vsp1_hgt_read(hgt, VI6_HGT_HISTO(m, n)); > + > + vsp1_histogram_buffer_complete(&hgt->histo, buf, HGT_DATA_SIZE); > +} > + > +/* ------------------------------------------------------------------------ > + * Controls > + */ > + > +#define V4L2_CID_VSP1_HGT_HUE_AREAS (V4L2_CID_USER_BASE | 0x1001) > + > +static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt, > + ctrls.handler); > + int m, n; m and n take positive values only, you can make them unsigned int. Additionally, I would use i and j here, as m and n have a specific hardware meaning. > + bool ok; > + > + /* > + * Make sure values meet one of two possible HW constrains > + * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U > + * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L Wouldn't it be better to test for 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U unconditionally, and then for (OL <= OU || 5U <= OL) ? You could possibly fold the test and fix loops in a single operation then. > + */ > + for (m = 0; m <= 1; m++) { > + ok = true; > + for (n = 0; n < HGT_NUM_HUE_AREAS - 1; n++) { > + if (ctrl->p_new.p_u8[(m + n + 0) % HGT_NUM_HUE_AREAS] > > m + n + 0 is always < HGT_NUM_HUE_AREAS so you can remove the % HGT_NUM_HUE_AREAS here. You could also shorten a few lines if you declared const u8 *value = ctrl->p_new.p_u8; at the beginning of the function. > + ctrl->p_new.p_u8[(m + n + 1) % HGT_NUM_HUE_AREAS]) > + ok = false; > + } > + if (ok) > + break; > + } > + > + /* Values do not match HW, adjust to a valid setting */ You can spell out HW as hardware :-) s/a valid setting/valid settings./ > + if (!ok) { > + for (n = 0; n < HGT_NUM_HUE_AREAS - 1; n++) { > + if (ctrl->p_new.p_u8[n] > ctrl->p_new.p_u8[n+1]) > + ctrl->p_new.p_u8[n] = ctrl->p_new.p_u8[n+1]; > + } > + } > + > + for (n = 0; n < HGT_NUM_HUE_AREAS; n++) > + hgt->hue_area[n] = ctrl->p_new.p_u8[n]; With hue_area declared as u8 unsigned of unsigned int (assuming it makes sense, please see below) you could use a memcpy. > + > + return 0; > +} > + > +static const struct v4l2_ctrl_ops hgt_hue_areas_ctrl_ops = { > + .s_ctrl = hgt_hue_areas_s_ctrl, > +}; > + > +static const struct v4l2_ctrl_config hgt_hue_areas = { > + .ops = &hgt_hue_areas_ctrl_ops, > + .id = V4L2_CID_VSP1_HGT_HUE_AREAS, > + .name = "Boundary Values for Hue Area", > + .type = V4L2_CTRL_TYPE_U8, > + .min = 0, > + .max = 255, > + .def = 0, > + .step = 1, > + .dims = { 12 }, > +}; > + > + > +/* ------------------------------------------------------------------------ > + * V4L2 Subdevice Operations > + */ [snip] > +static const struct v4l2_subdev_ops hgt_ops = { > + .pad = &hgt_pad_ops, > +}; Except for the list of formats that differ between the two entityes, the subdev operations are identical for the HGO and HGT. I've sent you a patch that moves the implementation from vsp1_hgo.c to vsp1_histo.c, could you rebase this patch on top of it (and test the result) ? > +/* ------------------------------------------------------------------------ > + * VSP1 Entity Operations > + */ > + > +static void hgt_configure(struct vsp1_entity *entity, > + struct vsp1_pipeline *pipe, > + struct vsp1_dl_list *dl, bool full) > +{ > + struct vsp1_hgt *hgt = to_hgt(&entity->subdev); > + struct v4l2_rect *compose; > + struct v4l2_rect *crop; > + unsigned int hratio; > + unsigned int vratio; > + uint8_t lower, upper; Within the kernel you can use u8 instead of uint8_t. Could you please declare the two variables on separate lines ? > + int i; i only takes positive values, you can make it unsigned int. > + > + if (!full) > + return; > + > + crop = vsp1_entity_get_pad_selection(entity, entity->config, > + HGT_PAD_SINK, V4L2_SEL_TGT_CROP); > + compose = vsp1_entity_get_pad_selection(entity, entity->config, > + HGT_PAD_SINK, > + V4L2_SEL_TGT_COMPOSE); > + > + vsp1_hgt_write(hgt, dl, VI6_HGT_REGRST, VI6_HGT_REGRST_RCLEA); > + > + vsp1_hgt_write(hgt, dl, VI6_HGT_OFFSET, > + (crop->left << VI6_HGT_OFFSET_HOFFSET_SHIFT) | > + (crop->top << VI6_HGT_OFFSET_VOFFSET_SHIFT)); > + vsp1_hgt_write(hgt, dl, VI6_HGT_SIZE, > + (crop->width << VI6_HGT_SIZE_HSIZE_SHIFT) | > + (crop->height << VI6_HGT_SIZE_VSIZE_SHIFT)); > + > + mutex_lock(hgt->ctrls.handler.lock); > + for (i = 0; i < 6; i++) { > + lower = hgt->hue_area[i*2 + 0]; > + upper = hgt->hue_area[i*2 + 1]; > + vsp1_hgt_write(hgt, dl, VI6_HGT_HUE_AREA(i), > + (lower << VI6_HGT_HUE_AREA_LOWER_SHIFT) | > + (upper << VI6_HGT_HUE_AREA_UPPER_SHIFT)); > + } > + mutex_unlock(hgt->ctrls.handler.lock); > + > + hratio = crop->width * 2 / compose->width / 3; > + vratio = crop->height * 2 / compose->height / 3; > + vsp1_hgt_write(hgt, dl, VI6_HGT_MODE, > + (hratio << VI6_HGT_MODE_HRATIO_SHIFT) | > + (vratio << VI6_HGT_MODE_VRATIO_SHIFT)); > +} > + > +static void hgt_destroy(struct vsp1_entity *entity) > +{ > + struct vsp1_hgt *hgt = to_hgt(&entity->subdev); > + > + vsp1_histogram_cleanup(&hgt->histo); > +} > + > +static const struct vsp1_entity_operations hgt_entity_ops = { > + .configure = hgt_configure, > + .destroy = hgt_destroy, > +}; > + > +/* ------------------------------------------------------------------------ > + * Initialization and Cleanup > + */ > + > +struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1) > +{ > + struct vsp1_hgt *hgt; > + int i, ret; i takes positive a values only, you can declare it as an unsigned int. > + > + hgt = devm_kzalloc(vsp1->dev, sizeof(*hgt), GFP_KERNEL); > + if (hgt == NULL) > + return ERR_PTR(-ENOMEM); > + > + hgt->entity.ops = &hgt_entity_ops; > + hgt->entity.type = VSP1_ENTITY_HGT; > + > + ret = vsp1_entity_init(vsp1, &hgt->entity, "hgt", 2, &hgt_ops, > + MEDIA_ENT_F_PROC_VIDEO_STATISTICS); > + if (ret < 0) > + return ERR_PTR(ret); > + > + /* Initialize the control handler. */ > + for (i = 0; i < HGT_NUM_HUE_AREAS; i++) > + hgt->hue_area[i] = hgt_hue_areas.def; The right way to do this is to call v4l2_ctrl_handler_setup() at the end of the function. I need to fix a few locations in the driver where the values are initialized manually. > + v4l2_ctrl_handler_init(&hgt->ctrls.handler, 12); s/12/1/ > + hgt->ctrls.hue_areas = v4l2_ctrl_new_custom(&hgt->ctrls.handler, > + &hgt_hue_areas, NULL); > + hgt->entity.subdev.ctrl_handler = &hgt->ctrls.handler; > + > + /* Initialize the video device and queue for statistics data. */ > + ret = vsp1_histogram_init(vsp1, &hgt->histo, hgt->entity.subdev.name, > + HGT_DATA_SIZE, V4L2_META_FMT_VSP1_HGT); > + if (ret < 0) { > + vsp1_entity_destroy(&hgt->entity); > + return ERR_PTR(ret); > + } > + > + return hgt; > +} > diff --git a/drivers/media/platform/vsp1/vsp1_hgt.h > b/drivers/media/platform/vsp1/vsp1_hgt.h new file mode 100644 > index 0000000..a2f1eae > --- /dev/null > +++ b/drivers/media/platform/vsp1/vsp1_hgt.h > @@ -0,0 +1,51 @@ > +/* > + * vsp1_hgt.h -- R-Car VSP1 Histogram Generator 2D > + * > + * Copyright (C) 2016 Renesas Electronics Corporation > + * > + * Contact: Niklas Söderlund (niklas.soderlund@ragnatech.se) > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#ifndef __VSP1_HGT_H__ > +#define __VSP1_HGT_H__ > + > +#include <media/media-entity.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-subdev.h> > + > +#include "vsp1_entity.h" > +#include "vsp1_histo.h" > + > +struct vsp1_device; > + > +#define HGT_PAD_SINK 0 > +#define HGT_PAD_SOURCE 1 > + > +#define HGT_NUM_HUE_AREAS 12 Technically speaking there are 6 areas. I would thus define HGT_NUM_HUE_AREAS as equal to 6, and either use HGT_NUM_HUE_AREAS * 2 or modify the code to process the lower and upper boundaries separately. > +struct vsp1_hgt { > + struct vsp1_entity entity; > + struct vsp1_histogram histo; > + > + struct { > + struct v4l2_ctrl_handler handler; > + struct v4l2_ctrl *hue_areas; You don't need to store the hue_areas control in the vsp1_hgt structure, the value is only used in vsp1_hgt_create() where it can be stored in a local variable. You can thus get rid of the ctrls structure and just add a struct v4l2_ctrl_handler ctrls field. > + } ctrls; > + > + unsigned int hue_area[HGT_NUM_HUE_AREAS]; Shouldn't this be called hue_areas ? Using u8 would save a bit of memory, but possibly at the expense of the .text size. Could you check that ? > +}; > + > + A single blank line will do. > +static inline struct vsp1_hgt *to_hgt(struct v4l2_subdev *subdev) > +{ > + return container_of(subdev, struct vsp1_hgt, entity.subdev); > +} > + > +struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1); > +void vsp1_hgt_frame_end(struct vsp1_entity *hgt); > + > +#endif /* __VSP1_HGT_H__ */ [snip] -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] v4l: vsp1: Add HGT support 2016-09-05 15:43 ` Laurent Pinchart @ 2016-09-05 15:57 ` Geert Uytterhoeven 2016-09-05 18:24 ` Laurent Pinchart 2016-09-06 6:45 ` Niklas Söderlund 1 sibling, 1 reply; 9+ messages in thread From: Geert Uytterhoeven @ 2016-09-05 15:57 UTC (permalink / raw) To: Laurent Pinchart Cc: Niklas Söderlund, Linux-Renesas, Linux Media Mailing List, Jonathan Corbet, Mauro Carvalho Chehab, Sakari Ailus, Hans Verkuil Hi Laurent, On Mon, Sep 5, 2016 at 5:43 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> + for (n = 0; n < 6; n++) > > Nitpicking, the driver uses pre-increment in for loops (++n), not post- > increment. This used to be a best-practice rule in C++, where pre-increment > can be faster for non-native types (see http://antonym.org/2008/05/stl-iterators-and-performance.html for instance). I'm not sure if that's still > relevant, but I've taken the habit of using the pre-increment operator in for > loops, and that's what the rest of this driver does. This comment applies to > all other locations in this file. <surprised> Didn't know we used C++ and operator overloading in the kernel... </surprised> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] v4l: vsp1: Add HGT support 2016-09-05 15:57 ` Geert Uytterhoeven @ 2016-09-05 18:24 ` Laurent Pinchart 0 siblings, 0 replies; 9+ messages in thread From: Laurent Pinchart @ 2016-09-05 18:24 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Niklas Söderlund, Linux-Renesas, Linux Media Mailing List, Jonathan Corbet, Mauro Carvalho Chehab, Sakari Ailus, Hans Verkuil On Monday 05 Sep 2016 17:57:11 Geert Uytterhoeven wrote: > On Mon, Sep 5, 2016 at 5:43 PM, Laurent Pinchart wrote: > >> + for (n = 0; n < 6; n++) > > > > Nitpicking, the driver uses pre-increment in for loops (++n), not post- > > increment. This used to be a best-practice rule in C++, where > > pre-increment can be faster for non-native types (see > > http://antonym.org/2008/05/stl-iterators-and-performance.html for > > instance). I'm not sure if that's still relevant, but I've taken the > > habit of using the pre-increment operator in for loops, and that's what > > the rest of this driver does. This comment applies to all other locations > > in this file. > > <surprised> > Didn't know we used C++ and operator overloading in the kernel... > </surprised> Really ? Where were you when we decided to switch to C++ ? :-) On a more serious note, as I've explained, the *style* comes from a best practice rule in C++. This obviously makes no difference whatsoever in C, nor does it in C++ for integer types, it's only a matter of consistency with the rest of the driver. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] v4l: vsp1: Add HGT support 2016-09-05 15:43 ` Laurent Pinchart 2016-09-05 15:57 ` Geert Uytterhoeven @ 2016-09-06 6:45 ` Niklas Söderlund 1 sibling, 0 replies; 9+ messages in thread From: Niklas Söderlund @ 2016-09-06 6:45 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-renesas-soc, linux-media, corbet, mchehab, sakari.ailus, hans.verkuil Hi Laurent, Thanks for your review. On 2016-09-05 18:43:58 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Friday 02 Sep 2016 15:47:14 Niklas Söderlund wrote: > > The HGT is a Histogram Generator Two-Dimensions. It computes a weighted > > frequency histograms for hue and saturation areas over a configurable > > region of the image with optional subsampling. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/vsp1/Makefile | 2 +- > > drivers/media/platform/vsp1/vsp1.h | 3 + > > drivers/media/platform/vsp1/vsp1_drv.c | 32 +- > > drivers/media/platform/vsp1/vsp1_entity.c | 33 +- > > drivers/media/platform/vsp1/vsp1_entity.h | 1 + > > drivers/media/platform/vsp1/vsp1_hgt.c | 495 +++++++++++++++++++++++++++ > > drivers/media/platform/vsp1/vsp1_hgt.h | 51 +++ > > drivers/media/platform/vsp1/vsp1_pipe.c | 16 + > > drivers/media/platform/vsp1/vsp1_pipe.h | 2 + > > drivers/media/platform/vsp1/vsp1_regs.h | 9 + > > drivers/media/platform/vsp1/vsp1_video.c | 10 +- > > 11 files changed, 638 insertions(+), 16 deletions(-) > > create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c > > create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h > > [snip] > > > diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c > > b/drivers/media/platform/vsp1/vsp1_hgt.c new file mode 100644 > > index 0000000..c43373d > > --- /dev/null > > +++ b/drivers/media/platform/vsp1/vsp1_hgt.c > > @@ -0,0 +1,495 @@ > > +/* > > + * vsp1_hgt.c -- R-Car VSP1 Histogram Generator 2D > > + * > > + * Copyright (C) 2016 Renesas Electronics Corporation > > + * > > + * Contact: Niklas Söderlund (niklas.soderlund@ragnatech.se) > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/gfp.h> > > + > > +#include <media/v4l2-subdev.h> > > +#include <media/videobuf2-vmalloc.h> > > + > > +#include "vsp1.h" > > +#include "vsp1_dl.h" > > +#include "vsp1_hgt.h" > > + > > +#define HGT_MIN_SIZE 4U > > +#define HGT_MAX_SIZE 8192U > > +#define HGT_DATA_SIZE ((2 + 6 + 6 * 32) * 4) > > + > > +/* ------------------------------------------------------------------------ > > + * Device Access > > + */ > > + > > +static inline u32 vsp1_hgt_read(struct vsp1_hgt *hgt, u32 reg) > > +{ > > + return vsp1_read(hgt->entity.vsp1, reg); > > +} > > + > > +static inline void vsp1_hgt_write(struct vsp1_hgt *hgt, struct vsp1_dl_list > > *dl, > > + u32 reg, u32 data) > > +{ > > + vsp1_dl_list_write(dl, reg, data); > > +} > > + > > +/* ------------------------------------------------------------------------ > > + * Frame End Handler > > + */ > > + > > +void vsp1_hgt_frame_end(struct vsp1_entity *entity) > > +{ > > + struct vsp1_hgt *hgt = to_hgt(&entity->subdev); > > + struct vsp1_histogram_buffer *buf; > > + unsigned int m, n; > > + u32 *data; > > + > > + buf = vsp1_histogram_buffer_get(&hgt->histo); > > + if (!buf) > > + return; > > + > > + data = buf->addr; > > + > > + *data++ = vsp1_hgt_read(hgt, VI6_HGT_MAXMIN); > > + *data++ = vsp1_hgt_read(hgt, VI6_HGT_SUM); > > + > > + for (n = 0; n < 6; n++) > > Nitpicking, the driver uses pre-increment in for loops (++n), not post- > increment. This used to be a best-practice rule in C++, where pre-increment > can be faster for non-native types (see http://antonym.org/2008/05/stl-iterators-and-performance.html for instance). I'm not sure if that's still > relevant, but I've taken the habit of using the pre-increment operator in for > loops, and that's what the rest of this driver does. This comment applies to > all other locations in this file. Will update this for v2. > > > + *data++ = vsp1_hgt_read(hgt, VI6_HGT_HUE_AREA(n)); > > As commented on patch 1/2, I don't think this is needed. Userspace has > configured the hue areas, it should already have access to this information. > Note that if you wanted to keep this code you would need to synchronize it > with the .s_ctrl() handler to avoid race conditions. That's not trivial, and > in my opinion not needed. Will drop the hue area configuration in v2. > > > + for (m = 0; m < 6; m++) > > + for (n = 0; n < 32; n++) > > + *data++ = vsp1_hgt_read(hgt, VI6_HGT_HISTO(m, n)); > > + > > + vsp1_histogram_buffer_complete(&hgt->histo, buf, HGT_DATA_SIZE); > > +} > > + > > +/* ------------------------------------------------------------------------ > > + * Controls > > + */ > > + > > +#define V4L2_CID_VSP1_HGT_HUE_AREAS (V4L2_CID_USER_BASE | 0x1001) > > + > > +static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt, > > + ctrls.handler); > > + int m, n; > > m and n take positive values only, you can make them unsigned int. > Additionally, I would use i and j here, as m and n have a specific hardware > meaning. Will fix. > > > + bool ok; > > + > > + /* > > + * Make sure values meet one of two possible HW constrains > > + * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= > 5U > > + * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= > 0L > > Wouldn't it be better to test for 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L > <= 4U <= 5L <= 5U unconditionally, and then for (OL <= OU || 5U <= OL) ? You > could possibly fold the test and fix loops in a single operation then. Will update for v2, good suggestion. > > > + */ > > + for (m = 0; m <= 1; m++) { > > + ok = true; > > + for (n = 0; n < HGT_NUM_HUE_AREAS - 1; n++) { > > + if (ctrl->p_new.p_u8[(m + n + 0) % HGT_NUM_HUE_AREAS] > > > > > m + n + 0 is always < HGT_NUM_HUE_AREAS so you can remove the % > HGT_NUM_HUE_AREAS here. > > You could also shorten a few lines if you declared > > const u8 *value = ctrl->p_new.p_u8; > > at the beginning of the function. > Will fix for in v2. > > + ctrl->p_new.p_u8[(m + n + 1) % HGT_NUM_HUE_AREAS]) > > + ok = false; > > + } > > + if (ok) > > + break; > > + } > > + > > + /* Values do not match HW, adjust to a valid setting */ > > You can spell out HW as hardware :-) > > s/a valid setting/valid settings./ Thanks. > > > + if (!ok) { > > + for (n = 0; n < HGT_NUM_HUE_AREAS - 1; n++) { > > + if (ctrl->p_new.p_u8[n] > ctrl->p_new.p_u8[n+1]) > > + ctrl->p_new.p_u8[n] = ctrl->p_new.p_u8[n+1]; > > + } > > + } > > + > > + for (n = 0; n < HGT_NUM_HUE_AREAS; n++) > > + hgt->hue_area[n] = ctrl->p_new.p_u8[n]; > > With hue_area declared as u8 unsigned of unsigned int (assuming it makes > sense, please see below) you could use a memcpy. I be happy to do this if declaring hue_area as u8 make sens, but IMHO i think a memcpy here will decrease the readability of the code. > > > + > > + return 0; > > +} > > + > > +static const struct v4l2_ctrl_ops hgt_hue_areas_ctrl_ops = { > > + .s_ctrl = hgt_hue_areas_s_ctrl, > > +}; > > + > > +static const struct v4l2_ctrl_config hgt_hue_areas = { > > + .ops = &hgt_hue_areas_ctrl_ops, > > + .id = V4L2_CID_VSP1_HGT_HUE_AREAS, > > + .name = "Boundary Values for Hue Area", > > + .type = V4L2_CTRL_TYPE_U8, > > + .min = 0, > > + .max = 255, > > + .def = 0, > > + .step = 1, > > + .dims = { 12 }, > > +}; > > + > > + > > +/* ------------------------------------------------------------------------ > > + * V4L2 Subdevice Operations > > + */ > > [snip] > > > +static const struct v4l2_subdev_ops hgt_ops = { > > + .pad = &hgt_pad_ops, > > +}; > > Except for the list of formats that differ between the two entityes, the > subdev operations are identical for the HGO and HGT. I've sent you a patch > that moves the implementation from vsp1_hgo.c to vsp1_histo.c, could you > rebase this patch on top of it (and test the result) ? Thanks for the patch, will rebase v2 on top of it. > > > +/* ------------------------------------------------------------------------ > > + * VSP1 Entity Operations > > + */ > > + > > +static void hgt_configure(struct vsp1_entity *entity, > > + struct vsp1_pipeline *pipe, > > + struct vsp1_dl_list *dl, bool full) > > +{ > > + struct vsp1_hgt *hgt = to_hgt(&entity->subdev); > > + struct v4l2_rect *compose; > > + struct v4l2_rect *crop; > > + unsigned int hratio; > > + unsigned int vratio; > > + uint8_t lower, upper; > > Within the kernel you can use u8 instead of uint8_t. > Will update in v2. > Could you please declare the two variables on separate lines ? > Will update in v2. I assume this is to keep consistent with other parts of the driver? If so I will update all occurrences in the HGT driver to only declare one variable per line. > > + int i; > > i only takes positive values, you can make it unsigned int. Will update in v2. > > > + > > + if (!full) > > + return; > > + > > + crop = vsp1_entity_get_pad_selection(entity, entity->config, > > + HGT_PAD_SINK, V4L2_SEL_TGT_CROP); > > + compose = vsp1_entity_get_pad_selection(entity, entity->config, > > + HGT_PAD_SINK, > > + V4L2_SEL_TGT_COMPOSE); > > + > > + vsp1_hgt_write(hgt, dl, VI6_HGT_REGRST, VI6_HGT_REGRST_RCLEA); > > + > > + vsp1_hgt_write(hgt, dl, VI6_HGT_OFFSET, > > + (crop->left << VI6_HGT_OFFSET_HOFFSET_SHIFT) | > > + (crop->top << VI6_HGT_OFFSET_VOFFSET_SHIFT)); > > + vsp1_hgt_write(hgt, dl, VI6_HGT_SIZE, > > + (crop->width << VI6_HGT_SIZE_HSIZE_SHIFT) | > > + (crop->height << VI6_HGT_SIZE_VSIZE_SHIFT)); > > + > > + mutex_lock(hgt->ctrls.handler.lock); > > + for (i = 0; i < 6; i++) { > > + lower = hgt->hue_area[i*2 + 0]; > > + upper = hgt->hue_area[i*2 + 1]; > > + vsp1_hgt_write(hgt, dl, VI6_HGT_HUE_AREA(i), > > + (lower << VI6_HGT_HUE_AREA_LOWER_SHIFT) | > > + (upper << VI6_HGT_HUE_AREA_UPPER_SHIFT)); > > + } > > + mutex_unlock(hgt->ctrls.handler.lock); > > + > > + hratio = crop->width * 2 / compose->width / 3; > > + vratio = crop->height * 2 / compose->height / 3; > > + vsp1_hgt_write(hgt, dl, VI6_HGT_MODE, > > + (hratio << VI6_HGT_MODE_HRATIO_SHIFT) | > > + (vratio << VI6_HGT_MODE_VRATIO_SHIFT)); > > +} > > + > > +static void hgt_destroy(struct vsp1_entity *entity) > > +{ > > + struct vsp1_hgt *hgt = to_hgt(&entity->subdev); > > + > > + vsp1_histogram_cleanup(&hgt->histo); > > +} > > + > > +static const struct vsp1_entity_operations hgt_entity_ops = { > > + .configure = hgt_configure, > > + .destroy = hgt_destroy, > > +}; > > + > > +/* ------------------------------------------------------------------------ > > + * Initialization and Cleanup > > + */ > > + > > +struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1) > > +{ > > + struct vsp1_hgt *hgt; > > + int i, ret; > > i takes positive a values only, you can declare it as an unsigned int. > Will fix in v2. > > + > > + hgt = devm_kzalloc(vsp1->dev, sizeof(*hgt), GFP_KERNEL); > > + if (hgt == NULL) > > + return ERR_PTR(-ENOMEM); > > + > > + hgt->entity.ops = &hgt_entity_ops; > > + hgt->entity.type = VSP1_ENTITY_HGT; > > + > > + ret = vsp1_entity_init(vsp1, &hgt->entity, "hgt", 2, &hgt_ops, > > + MEDIA_ENT_F_PROC_VIDEO_STATISTICS); > > + if (ret < 0) > > + return ERR_PTR(ret); > > + > > + /* Initialize the control handler. */ > > + for (i = 0; i < HGT_NUM_HUE_AREAS; i++) > > + hgt->hue_area[i] = hgt_hue_areas.def; > > The right way to do this is to call v4l2_ctrl_handler_setup() at the end of > the function. I need to fix a few locations in the driver where the values are > initialized manually. Ohh, did not know that, will fix. > > > + v4l2_ctrl_handler_init(&hgt->ctrls.handler, 12); > > s/12/1/ Ops, thanks for spotting this. > > > + hgt->ctrls.hue_areas = v4l2_ctrl_new_custom(&hgt->ctrls.handler, > > + &hgt_hue_areas, NULL); > > + hgt->entity.subdev.ctrl_handler = &hgt->ctrls.handler; > > + > > + /* Initialize the video device and queue for statistics data. */ > > + ret = vsp1_histogram_init(vsp1, &hgt->histo, hgt->entity.subdev.name, > > + HGT_DATA_SIZE, V4L2_META_FMT_VSP1_HGT); > > + if (ret < 0) { > > + vsp1_entity_destroy(&hgt->entity); > > + return ERR_PTR(ret); > > + } > > + > > + return hgt; > > +} > > diff --git a/drivers/media/platform/vsp1/vsp1_hgt.h > > b/drivers/media/platform/vsp1/vsp1_hgt.h new file mode 100644 > > index 0000000..a2f1eae > > --- /dev/null > > +++ b/drivers/media/platform/vsp1/vsp1_hgt.h > > @@ -0,0 +1,51 @@ > > +/* > > + * vsp1_hgt.h -- R-Car VSP1 Histogram Generator 2D > > + * > > + * Copyright (C) 2016 Renesas Electronics Corporation > > + * > > + * Contact: Niklas Söderlund (niklas.soderlund@ragnatech.se) > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + */ > > +#ifndef __VSP1_HGT_H__ > > +#define __VSP1_HGT_H__ > > + > > +#include <media/media-entity.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-subdev.h> > > + > > +#include "vsp1_entity.h" > > +#include "vsp1_histo.h" > > + > > +struct vsp1_device; > > + > > +#define HGT_PAD_SINK 0 > > +#define HGT_PAD_SOURCE 1 > > + > > +#define HGT_NUM_HUE_AREAS 12 > > Technically speaking there are 6 areas. I would thus define HGT_NUM_HUE_AREAS > as equal to 6, and either use HGT_NUM_HUE_AREAS * 2 or modify the code to > process the lower and upper boundaries separately. Will fix in v2. > > > +struct vsp1_hgt { > > + struct vsp1_entity entity; > > + struct vsp1_histogram histo; > > + > > + struct { > > + struct v4l2_ctrl_handler handler; > > + struct v4l2_ctrl *hue_areas; > > You don't need to store the hue_areas control in the vsp1_hgt structure, the > value is only used in vsp1_hgt_create() where it can be stored in a local > variable. You can thus get rid of the ctrls structure and just add a struct > v4l2_ctrl_handler ctrls field. > Will fix in v2. > > + } ctrls; > > + > > + unsigned int hue_area[HGT_NUM_HUE_AREAS]; > > Shouldn't this be called hue_areas ? > > Using u8 would save a bit of memory, but possibly at the expense of the .text > size. Could you check that ? Will check and update if it makes sens. > > > +}; > > + > > + > > A single blank line will do. Yes. > > > +static inline struct vsp1_hgt *to_hgt(struct v4l2_subdev *subdev) > > +{ > > + return container_of(subdev, struct vsp1_hgt, entity.subdev); > > +} > > + > > +struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1); > > +void vsp1_hgt_frame_end(struct vsp1_entity *hgt); > > + > > +#endif /* __VSP1_HGT_H__ */ > > [snip] > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-09-06 6:45 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-02 13:47 [PATCH 0/2] v4l: vsp1: Add HGT support Niklas Söderlund 2016-09-02 13:47 ` [PATCH 1/2] v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine Niklas Söderlund 2016-09-05 13:05 ` Laurent Pinchart 2016-09-05 13:34 ` Niklas Söderlund 2016-09-02 13:47 ` [PATCH 2/2] v4l: vsp1: Add HGT support Niklas Söderlund 2016-09-05 15:43 ` Laurent Pinchart 2016-09-05 15:57 ` Geert Uytterhoeven 2016-09-05 18:24 ` Laurent Pinchart 2016-09-06 6:45 ` Niklas Söderlund
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox