Re: [PATCH v0 1/5] Change/add data structures for SRG mouse reporting

[Jiri Slaby <jirislaby@kernel.org>]

On 26. 06. 20, 9:22, Tammo Block wrote:
> This extends the data structures and defines needed for additional
> mouse reporting features. If URXVT and UTF8 reports should be supported
> in the future, vc_proto_mouse would need another bit.

Just a global remark: I have just rewritten a large part of the vt code.
You might need to rebase on the top of tty-next...

> Signed-off-by: Tammo Block <tammo.block@gmail.com>
> ---
>  include/linux/console_struct.h |  3 ++-
>  include/uapi/linux/tiocl.h     | 24 ++++++++++++++++--------
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/console_struct.h b/include/linux/console_struct.h
> index 24d4c16e3ae0..cfb581eb8b82 100644
> --- a/include/linux/console_struct.h
> +++ b/include/linux/console_struct.h
> @@ -122,7 +122,8 @@ struct vc_data {
>  	unsigned int	vc_priv		: 3;
>  	unsigned int	vc_need_wrap	: 1;
>  	unsigned int	vc_can_do_color	: 1;
> -	unsigned int	vc_report_mouse : 2;
> +	unsigned int	vc_report_mouse : 3;
> +	unsigned int	vc_proto_mouse  : 1;

bitfields... The generated code is so bad (it does masks, shifts and
ANDs in assembly), so please avoid them and define the new members the
standard way. I.e. unsigned char if 8 bits are enough, bool for one bit
and so on. The price is larger structure, but we can afford 5 more bits
per virt console, I think. Provided there is a very limited number them
-- this is not "struct page" after all :).

>  	unsigned char	vc_utf		: 1;	/* Unicode UTF-8 encoding */
>  	unsigned char	vc_utf_count;
>  		 int	vc_utf_char;
> diff --git a/include/uapi/linux/tiocl.h b/include/uapi/linux/tiocl.h
> index b32acc229024..df27608648f7 100644
> --- a/include/uapi/linux/tiocl.h
> +++ b/include/uapi/linux/tiocl.h
> @@ -3,13 +3,17 @@
>  #define _LINUX_TIOCL_H
>  
>  #define TIOCL_SETSEL	2	/* set a selection */
> -#define 	TIOCL_SELCHAR	0	/* select characters */
> -#define 	TIOCL_SELWORD	1	/* select whole words */
> -#define 	TIOCL_SELLINE	2	/* select whole lines */
> -#define 	TIOCL_SELPOINTER	3	/* show the pointer */
> -#define 	TIOCL_SELCLEAR	4	/* clear visibility of selection */
> -#define 	TIOCL_SELMOUSEREPORT	16	/* report beginning of selection */
> -#define 	TIOCL_SELBUTTONMASK	15	/* button mask for report */
> +#define		TIOCL_SELCHAR	0	/* select characters */
> +#define		TIOCL_SELWORD	1	/* select whole words */
> +#define		TIOCL_SELLINE	2	/* select whole lines */
> +#define		TIOCL_SELPOINTER	3	/* show the pointer */
> +#define		TIOCL_SELCLEAR	4	/* clear visibility of selection */
> +#define		TIOCL_SELMOUSEREPORT	16	/* send X10 mouse report */
> +#define		TIOCL_SELBUTTONMASK	15	/* button mask for X10 report */
> +#define		TIOCL_SELSRGREPORT	32	/* send SRG mouse report */
> +#define		TIOCL_SELSRGRELEASE	128	/* SRG report is release event */
> +#define		TIOCL_SELSRGMASK	(255 << 8)	/* mask for SRG report */

This right shift of all of them feels like too much right. Why do you do
it? And if you do it, do it in a separate patch. It makes this one
complicated to review.

> @@ -28,7 +32,11 @@ struct tiocl_selection {
>  
>  /* these two don't return a value: they write it back in the type */
>  #define TIOCL_GETSHIFTSTATE	6	/* write shift state */
> -#define TIOCL_GETMOUSEREPORTING	7	/* write whether mouse event are reported */
> +#define TIOCL_GETMOUSEREPORTING	7	/* write which mouse event to be reported */
> +#define		TIOCL_REPORTBTNPRESS	1	/* report button press only    "9" */
> +#define		TIOCL_REPORTRELEASE	2	/* report press and release "1000" */
> +#define		TIOCL_REPORTDRAG	5	/* report drag events       "1002" */
> +#define		TIOCL_REPORTANYMOVE	6	/* report any movement      "1003" */

Dtto.

>  #define TIOCL_SETVESABLANK	10	/* set vesa blanking mode */
>  #define TIOCL_SETKMSGREDIRECT	11	/* restrict kernel messages to a vt */
>  #define TIOCL_GETFGCONSOLE	12	/* get foreground vt */

thanks,
-- 
js