Re: [PATCH] modetest: add the possibility to select the refresh frequency for a mode

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

Hi Vincent,

Thank you for the patch.

On Tuesday 07 January 2014 09:55:40 Vincent ABRIOU wrote:
> When mode is selected we only give the name of the mode as parameter.
> But sometime, two different modes have the same name but not
> the same vrefresh frequency.
> This patch give the possibility to select a mode by its name
> and optionally by its refresh frequency.
> 
> Signed-off-by: Vincent Abriou <vincent.abriou@st.com>
> ---
>  tests/modetest/modetest.c |   38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> index 51c4e6d..259d580 100644
> --- a/tests/modetest/modetest.c
> +++ b/tests/modetest/modetest.c
> @@ -693,6 +693,7 @@ struct pipe_arg {
>  	uint32_t crtc_id;
>  	char mode_str[64];
>  	char format_str[5];
> +	unsigned int refresh_freq;

What about calling it vrefresh to match the drm_mode field name ?

>  	unsigned int fourcc;
>  	drmModeModeInfo *mode;
>  	struct crtc *crtc;
> @@ -714,7 +715,8 @@ struct plane_arg {
>  };
> 
>  static drmModeModeInfo *
> -connector_find_mode(struct device *dev, uint32_t con_id, const char
> *mode_str)
> +connector_find_mode(struct device *dev, uint32_t con_id, const char
> *mode_str,
> +        const unsigned int refresh_freq)
>  {
>  	drmModeConnector *connector;
>  	drmModeModeInfo *mode;
> @@ -726,8 +728,16 @@ connector_find_mode(struct device *dev, uint32_t
> con_id, const char *mode_str)
> 
>  	for (i = 0; i < connector->count_modes; i++) {
>  		mode = &connector->modes[i];
> -		if (!strcmp(mode->name, mode_str))
> -			return mode;
> +		if (!strcmp(mode->name, mode_str)) {
> +			if (refresh_freq == 0)
> +				/* If the refresh frequency is not specified then return the
> +				 * first mode that match with the name */
> +				return mode;
> +			else if (mode->vrefresh == refresh_freq)
> +				/* Else, return the mode that match the name and the
> specified
> +				 * refresh frequency */
> +				return mode;
> +		}
>  	}
> 
>  	return NULL;
> @@ -789,7 +799,7 @@ static int pipe_find_crtc_and_mode(struct device *dev,
> struct pipe_arg *pipe)
> 
>  	for (i = 0; i < (int)pipe->num_cons; i++) {
>  		mode = connector_find_mode(dev, pipe->con_ids[i],
> -					   pipe->mode_str);
> +					   pipe->mode_str, pipe->refresh_freq);
>  		if (mode == NULL) {
>  			fprintf(stderr,
>  				"failed to find mode \"%s\" for connector %u\n",
> @@ -1059,8 +1069,8 @@ static void set_mode(struct device *dev, struct
> pipe_arg *pipes, unsigned int co if (pipe->mode == NULL)
>  			continue;
> 
> -		printf("setting mode %s@%s on connectors ",
> -		       pipe->mode_str, pipe->format_str);
> +		printf("setting mode %s-%dHz@%s on connectors ",
> +		       pipe->mode_str, pipe->mode->vrefresh, pipe->format_str);
>  		for (j = 0; j < pipe->num_cons; ++j)
>  			printf("%u, ", pipe->con_ids[j]);
>  		printf("crtc %d\n", pipe->crtc->crtc->crtc_id);
> @@ -1192,6 +1202,7 @@ static int parse_connector(struct pipe_arg *pipe,
> const char *arg) const char *p;
>  	char *endp;
> 
> +	pipe->refresh_freq = 0;
>  	pipe->crtc_id = (uint32_t)-1;
>  	strcpy(pipe->format_str, "XR24");
> 
> @@ -1226,11 +1237,22 @@ static int parse_connector(struct pipe_arg *pipe,
> const char *arg)
> 
>  	arg = endp + 1;
> 
> -	p = strchrnul(arg, '@');
> +	/* Check if the refresh frequency is specified */
> +	p = strchr(arg, '-');
> +	if (p == NULL)
> +		/* Else check if format is specified */
> +		p = strchrnul(arg, '@');

Seems we need a strpbrknul :-)

Another option would be

        /* Search for the vertical refresh frequency or the fourcc. */
        p = strpbrk(arg, ":@");
        if (p == NULL)
                p = arg + strlen(arg);

I'm not sure which one would be more efficient.

>  	len = min(sizeof pipe->mode_str - 1, (unsigned int)(p - arg));
>  	strncpy(pipe->mode_str, arg, len);
>  	pipe->mode_str[len] = '\0';
> 
> +	if (*p == '-') {
> +		pipe->refresh_freq = strtoul(p + 1, &endp, 10);
> +		arg = endp;
> +		/* Once done, check if format is specified */
> +		p = strchrnul(arg, '@');

I think you can replace the strchrnul with a p = endp. There can't be anything 
between the end of the refresh rate and the '@'.

> +	}
> +
>  	if (*p == '@') {
>  		strncpy(pipe->format_str, p + 1, 4);
>  		pipe->format_str[4] = '\0';
> @@ -1323,7 +1345,7 @@ static void usage(char *name)
> 
>  	fprintf(stderr, "\n Test options:\n\n");
>  	fprintf(stderr, "\t-P <crtc_id>:<w>x<h>[+<x>+<y>][*<scale>]
> [@<format>]\tset a plane\n");
> -	fprintf(stderr, "\t-s <connector_id>[,<connector_id>]
> [@<crtc_id>]:<mode>[@<format>]\tset a mode\n");
> +	fprintf(stderr, "\t-s <connector_id>[,<connector_id>]
> [@<crtc_id>]:<mode>[-<refresh_freq>][@<format>]\tset a mode\n");
>  	fprintf(stderr, "\t-v\ttest vsynced page flipping\n");
>  	fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n");
-- 
Regards,

Laurent Pinchart