Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Hans Verkuil <hverkuil@xs4all.nl>
To: Vandana BN <bnvandana@gmail.com>, linux-media@vger.kernel.org
Subject: Re: [PATCH v7] vivid: Add touch support
Date: Tue, 19 Nov 2019 10:23:26 +0100
Message-ID: <eb0aa9e6-65fe-a727-d358-70930790f641@xs4all.nl> (raw)
In-Reply-To: <00480290-054d-3b13-2ec5-6a17e8cf0603@xs4all.nl>

On 11/19/19 10:02 AM, Hans Verkuil wrote:
> Hi Vandana,
> 
> On 11/18/19 4:53 PM, Vandana BN wrote:
>> Support to emulate touch devices in vivid driver.
>> It generates touch patterns simulating, single tap, double tap, triple
> 
> No , after 'simulating'.
> 
>> tap, move from left to right, zoom in, zoom out, palm press simulating
>> large area being pressed on screen, and an invalid pattern of
> 
> Drop the 'invalid': it is not invalid, just very, very unusual.
> 
>> simulating 16 different simultaneous touch points.
>> All of these patterns are based on behavior of rmi_f54 driver.
> 
> The patterns aren't from that driver, but the generated values are.
> 
>>
>> Signed-off-by: Vandana BN <bnvandana@gmail.com>
>> ---
>> Changes since V6:
>> 	simplified touch pattern generation.
>> ---
>>  drivers/media/platform/vivid/Makefile         |   3 +-
>>  drivers/media/platform/vivid/vivid-core.c     | 164 +++++++++-
>>  drivers/media/platform/vivid/vivid-core.h     |  20 ++
>>  drivers/media/platform/vivid/vivid-ctrls.c    |  11 +
>>  .../platform/vivid/vivid-kthread-touch.c      | 181 +++++++++++
>>  .../platform/vivid/vivid-kthread-touch.h      |  13 +
>>  .../media/platform/vivid/vivid-touch-cap.c    | 286 ++++++++++++++++++
>>  .../media/platform/vivid/vivid-touch-cap.h    |  35 +++
>>  8 files changed, 700 insertions(+), 13 deletions(-)
>>  create mode 100644 drivers/media/platform/vivid/vivid-kthread-touch.c
>>  create mode 100644 drivers/media/platform/vivid/vivid-kthread-touch.h
>>  create mode 100644 drivers/media/platform/vivid/vivid-touch-cap.c
>>  create mode 100644 drivers/media/platform/vivid/vivid-touch-cap.h
> 
> <snip>
> 

I couldn't resist: here is a vivid_tch_buf_set that fills in the neighboring
pixels as well for more realism:

static void vivid_tch_buf_set(__s16 *tch_buf, struct v4l2_pix_format *f, int index)
{
        unsigned int x = index % f->width;
        unsigned int y = index / f->width;
        unsigned int offset = VIVID_MIN_PRESSURE;

        tch_buf[index] = offset + get_random_int() % 40;
        offset /= 2;
        if (x)
                tch_buf[index - 1] = offset + get_random_int() % 40;
        if (x < f->width - 1)
                tch_buf[index + 1] = offset + get_random_int() % 40;
        if (y)
                tch_buf[index - f->width] = offset + get_random_int() % 40;
        if (y < f->height - 1)
                tch_buf[index + f->width] = offset + get_random_int() % 40;
        offset /= 2;
        if (x && y)
                tch_buf[index - 1 - f->width] = offset + get_random_int() % 40;
        if (x < f->width - 1 && y)
                tch_buf[index + 1 - f->width] = offset + get_random_int() % 40;
        if (x && y < f->height - 1)
                tch_buf[index - 1 + f->width] = offset + get_random_int() % 40;
        if (x < f->width - 1 && y < f->height - 1)
                tch_buf[index + 1 + f->width] = offset + get_random_int() % 40;
}

The result now looks very close to the real hardware.

>> +void vivid_fillbuff_tch(struct vivid_dev *dev, struct vivid_buffer *buf)
>> +{
>> +	struct v4l2_pix_format *f = &dev->tch_format;
>> +	int size = f->width * f->height;
>> +	int index = 0, x, y, xstart, ystart;
>> +	unsigned int test_pattern, test_pat_idx, rand;
>> +
>> +	__s16 *tch_buf = vb2_plane_vaddr(&buf->vb.vb2_buf, 0);
>> +
>> +	buf->vb.sequence = dev->touch_cap_seq_count;
>> +	test_pattern = (buf->vb.sequence / 15) % TEST_CASE_MAX;
>> +	test_pat_idx = buf->vb.sequence % 15;
>> +
>> +	vivid_fill_buff_noise(tch_buf, size);
>> +
>> +	if (test_pat_idx >= 10)
> 
> The values 10 and 15 should be defines. That makes it easy to change later.
> 
> I think it would be better to change 15 to 16 and 10 to 12: 16 helps because
> in the corner case where the sequence counter will wrap around you will get
> a smooth transition. And 12 makes each sequence just a little bit longer,
> which makes it easier to see.
> 
>> +		return;
>> +
>> +	if (test_pat_idx == 0)
>> +		dev->tch_pat_random = get_random_int();
>> +	rand = dev->tch_pat_random;
>> +
>> +	switch (test_pattern) {
>> +	case SINGLE_TAP:
>> +		if (test_pat_idx == 5)
>> +			vivid_tch_buf_set(tch_buf, rand % size);
>> +		break;
>> +	case DOUBLE_TAP:
>> +		if (test_pat_idx == 3 || test_pat_idx == 6)
>> +			vivid_tch_buf_set(tch_buf, rand % size);
>> +		break;
>> +	case TRIPLE_TAP:
>> +		if (test_pat_idx == 3 || test_pat_idx == 6 || test_pat_idx == 9)
>> +			vivid_tch_buf_set(tch_buf, rand % size);
>> +		break;
>> +	case MOVE_LEFT_TO_RIGHT:
>> +		vivid_tch_buf_set(tch_buf,
>> +				  (rand % f->height) * f->width + test_pat_idx * (f->width / 10));
> 
> Line too long (same in similar cases below). It's easiest to break it off after the + like this:
> 
> 		vivid_tch_buf_set(tch_buf,
> 				  (rand % f->height) * f->width +
> 				  test_pat_idx * (f->width / 10));
> 
>> +		break;
>> +	case ZOOM_IN:
>> +		x = f->width / 2;
>> +		y = f->height / 2;
> 
> You need extra variables for the offset from the center:
> 
> 		offset_x = ((9 - test_pat_idx) * x) / 10;   (10 should be a define as discussed above)
> 		offset_y = ((9 - test_pat_idx) * y) / 10;
> 
>> +		if (x + test_pat_idx < f->width && y + test_pat_idx < f->height) {
> 
> Then this if can be dropped here as well.
> 
>> +			vivid_tch_buf_set(tch_buf, (x - test_pat_idx) + f->width * (y - test_pat_idx));
>> +			vivid_tch_buf_set(tch_buf, (x + test_pat_idx) + f->width * (y + test_pat_idx));
>> +		}
>> +		break;
>> +	case ZOOM_OUT:
>> +		x = f->width / 2;
>> +		y = f->height / 2;
>> +		index = min(x, y) - test_pat_idx - 1;> +		if (x + index >= f->width / 2 && y + index >= f->height / 2) {
> 
> Same here.
> 
>> +			vivid_tch_buf_set(tch_buf, (x - index) + f->width * (y - index));
>> +			vivid_tch_buf_set(tch_buf, (x + index) + f->width * (y + index));
>> +		}
>> +		break;
>> +	case PALM_PRESS:

For this palm press test and...

>> +		xstart = f->width - f->width / 4;
> 
> Just set this to 0, so the palm covers the bottom quarter.
> 
>> +		ystart = f->height - f->height / 4;
>> +		if (test_pat_idx != 5)
>> +			break;
>> +		for (x = xstart; x < f->width; x++)
>> +			for (y = ystart; y < f->height; y++)
>> +				vivid_tch_buf_set(tch_buf, x + f->width * y);
>> +		break;
>> +	case MULTIPLE_PRESS:

... the multiple press test you can drop the 'if (test_pat_idx != 5)' bit:
i.e. this pattern should be there for the full 10 frames.

The point of this pattern is to test something unusual, so it makes sense
to show these two patterns for a longer time.

Regards,

	Hans

>> +		/* 16 pressure points */
>> +		if (test_pat_idx != 5)
>> +			break;
>> +		for (y = 0; y < 4; y++) {
>> +			for (x = 0; x < 4; x++) {
>> +				ystart = (y * f->height) / 4 + f->height / 8;
>> +				xstart = (x * f->width) / 4 + f->width / 8;
>> +				vivid_tch_buf_set(tch_buf, ystart * f->width + xstart);
>> +			}
>> +		}
>> +		break;
>> +	}
>> +}
> 
> Regards,
> 
> 	Hans
> 
>> diff --git a/drivers/media/platform/vivid/vivid-touch-cap.h b/drivers/media/platform/vivid/vivid-touch-cap.h
>> new file mode 100644
>> index 000000000000..6e83ca919f07
>> --- /dev/null
>> +++ b/drivers/media/platform/vivid/vivid-touch-cap.h
>> @@ -0,0 +1,35 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * vivid-touch-cap.h - touch support functions.
>> + */
>> +#ifndef _VIVID_TOUCH_CAP_H_
>> +#define _VIVID_TOUCH_CAP_H_
>> +
>> +#define VIVID_TCH_HEIGHT	12
>> +#define VIVID_TCH_WIDTH		21
>> +#define VIVID_MIN_PRESSURE	180
>> +
>> +enum vivid_tch_test {
>> +	SINGLE_TAP,
>> +	DOUBLE_TAP,
>> +	TRIPLE_TAP,
>> +	MOVE_LEFT_TO_RIGHT,
>> +	ZOOM_IN,
>> +	ZOOM_OUT,
>> +	PALM_PRESS,
>> +	MULTIPLE_PRESS,
>> +	TEST_CASE_MAX
>> +};
>> +
>> +extern const struct vb2_ops vivid_touch_cap_qops;
>> +
>> +int vivid_enum_fmt_tch(struct file *file, void  *priv, struct v4l2_fmtdesc *f);
>> +int vivid_g_fmt_tch(struct file *file, void *priv, struct v4l2_format *f);
>> +int vivid_enum_input_tch(struct file *file, void *priv, struct v4l2_input *inp);
>> +int vivid_g_input_tch(struct file *file, void *priv, unsigned int *i);
>> +int vivid_s_input_tch(struct file *file, void *priv, unsigned int i);
>> +void vivid_fillbuff_tch(struct vivid_dev *dev, struct vivid_buffer *buf);
>> +int vivid_set_touch(struct vivid_dev *dev, unsigned int i);
>> +int vivid_g_parm_tch(struct file *file, void *priv,
>> +		     struct v4l2_streamparm *parm);
>> +#endif
>>
> 


  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <9719685b-1b71-17e0-f43a-efbb53592e27@xs4all.nl>
2019-11-18 15:53 ` Vandana BN
2019-11-19  9:02   ` Hans Verkuil
2019-11-19  9:23     ` Hans Verkuil [this message]
2019-11-19  9:55       ` Hans Verkuil
2019-11-25  4:23         ` [PATCH v8] " Vandana BN
2019-11-25 10:02           ` Hans Verkuil
2019-11-25 10:13             ` Hans Verkuil
2019-11-25 10:19             ` Hans Verkuil
2019-11-25 12:00             ` Hans Verkuil
2019-11-26 14:16               ` [PATCH v9] " Vandana BN

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=eb0aa9e6-65fe-a727-d358-70930790f641@xs4all.nl \
    --to=hverkuil@xs4all.nl \
    --cc=bnvandana@gmail.com \
    --cc=linux-media@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git