Re: [PATCHv2 2/2] v4l: vsp1: Add HGT support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

On Wednesday 07 Sep 2016 12:05:26 Niklas Söderlund wrote:
> On 2016-09-06 22:59:22 +0300, Laurent Pinchart wrote:
> > On Tuesday 06 Sep 2016 16:38:56 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    |  33 ++++-
> >>  drivers/media/platform/vsp1/vsp1_entity.c |  33 +++--
> >>  drivers/media/platform/vsp1/vsp1_entity.h |   1 +
> >>  drivers/media/platform/vsp1/vsp1_hgt.c    | 217 +++++++++++++++++++++++
> >>  drivers/media/platform/vsp1/vsp1_hgt.h    |  42 ++++++
> >>  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, 352 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..4e3f762
> >> --- /dev/null
> >> +++ b/drivers/media/platform/vsp1/vsp1_hgt.c
> > 
> > [snip]
> > 
> >> +/* ---------------------------------------------------------------------
> >> + * 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);
> >> +	u8 *value = ctrl->p_new.p_u8;
> > 
> > Nitpicking, I'd call the variable values.
> > 
> >> +	unsigned int i;
> >> +	bool ok = true;
> >> +
> >> +	/*
> >> +	 * Make sure values meet one of two possible hardware constrains
> > 
> > s/constrains/constraints./
> > 
> >> +	 * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <=
> >> 5U
> >> +	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <=
> >> 0L
> >> +	 */
> >> +
> >> +	if ((value[0] > value[1]) && (value[11] > value[0]))
> >> +		ok = false;
> >> +	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i)
> >> +		if (value[i] > value[i+1])
> >> +			ok = false;
> >> +
> >> +	/* Values do not match hardware, adjust to valid settings. */
> >> +	if (!ok) {
> >> +		for (i = 0; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
> >> +			if (value[i] > value[i+1])
> >> +				value[i] = value[i+1];
> >> +		}
> >> +	}
> > 
> > I'm afraid this won't work. Let's assume value[0] = 100, value[1] = 50,
> > value[2] = 25. The loop will unroll to
> > 
> > 	if (value[0] /* 100 */ > value[1] /* 50 */)
> > 		value[0] = value[1] /* 50 */;
> > 	
> > 	if (value[1] /* 50 */ > value[2] /* 25 */)
> > 		value[1] = value[2] /* 25 */;
> > 
> > You will end up with value[0] = 50, value[1] = 25, value[2] = 25, which
> > doesn't match the hardware constraints.
> > 
> > How about the following, which tests and fixes the values in a single
> > operation ?
> > 
> > static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > 	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
> > 					    ctrls);
> > 	u8 *values = ctrl->p_new.p_u8;
> > 	unsigned int i;
> > 	
> > 	/*
> > 	 * Adjust the values if they don't meet the hardware constraints:
> > 	 *
> > 	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U
> > 	 */
> > 	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
> > 		if (values[i] > values[i+1])
> > 			values[i+1] = values[i];
> > 	}
> > 	
> > 	/* 0L <= 0U or 5U <= 0L */
> > 	if (values[0] > values[1] && values[11] > values[0])
> > 		values[0] = values[1];
> > 	
> > 	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
> > 	
> > 	return 0;
> > }
> > 
> > I'm also beginning to wonder whether it wouldn't make sense to return
> > -EINVAL when the values don't match the constraints instead of trying to
> > fix them.
>
> I'm fine with either solution. I looked at a few other drivers and it
> seems the most common way is to correct the control value. But maybe in
> this case it's better to just return -EINVAL.
> 
> Let me know what you think and I will make it so and send a v3.

Given that fixed values would result in a different histogram that would 
likely be unusable by a userspace application that doesn't expect the control 
values to be changed (and if it did, it should set correct values to start 
with), I think -EINVAL is better. As Hans pointed out on IRC, this should be 
implemented in the .try_ctrl() handler.
 
> > > +	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
> > > +
> > > +	return 0;
> > > +}
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart