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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx)
> + *
> + * 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@xxxxxxxxxxxx)
> + *
> + * 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
