[PATCH weston v3] drm: port the drm backend to the new init api

[Quentin Glidic]

For now, I will just comment on the part I am not too happy with.

On 31/10/2015 12:08, Giulio Camuffo wrote:
> [snip]
> diff --git a/src/main.c b/src/main.c
> index 292f8e0..bde27ee 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -47,6 +47,8 @@
>   #include "git-version.h"
>   #include "version.h"
>
> +#include "compositor-drm.h"
> +
>   static struct wl_list child_process_list;
>   static struct weston_compositor *segv_compositor;
>
> @@ -666,12 +668,137 @@ load_backend_new(struct weston_compositor *compositor, const char *backend,
>   }
>
>   static int
> +parse_modeline(const char *s, struct weston_drm_backend_modeline *mode)
> +{
> +	char hsync[16];
> +	char vsync[16];
> +	float fclock;
> +
> +	mode->flags = 0;
> +
> +	if (sscanf(s, "%f %hd %hd %hd %hd %hd %hd %hd %hd %15s %15s",
> +		   &fclock,
> +		   &mode->hdisplay,
> +		   &mode->hsync_start,
> +		   &mode->hsync_end,
> +		   &mode->htotal,
> +		   &mode->vdisplay,
> +		   &mode->vsync_start,
> +		   &mode->vsync_end,
> +		   &mode->vtotal, hsync, vsync) != 11)
> +		return -1;
> +
> +	mode->clock = fclock * 1000;
> +	if (strcmp(hsync, "+hsync") == 0)
> +		mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_PHSYNC;
> +	else if (strcmp(hsync, "-hsync") == 0)
> +		mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_NHSYNC;
> +	else
> +		return -1;
> +
> +	if (strcmp(vsync, "+vsync") == 0)
> +		mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_PVSYNC;
> +	else if (strcmp(vsync, "-vsync") == 0)
> +		mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_NVSYNC;
> +	else
> +		return -1;
> +
> +	return 0;
> +}

As discussed on IRC, I think this part should stay in the drm backend. 
The rationale is that we probably want all libweston-based compositors 
to work with the exact same modeline format, with the same error 
messages and the same predictable behaviour.


> +static void
> +drm_configure_output(struct weston_compositor *c, const char *name,
> +		     struct weston_drm_backend_output_config *config)
> +{
> +	struct weston_config *wc = weston_compositor_get_user_data(c);
> +	struct weston_config_section *section;
> +	char *s;
> +	int scale;
> +
> +	section = weston_config_get_section(wc, "output", "name", name);
> +	weston_config_section_get_string(section, "mode", &s, "preferred");
> +	if (strcmp(s, "off") == 0)
> +		config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_OFF;
> +	else if (strcmp(s, "preferred") == 0)
> +		config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_PREFERRED;
> +	else if (strcmp(s, "current") == 0)
> +		config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_CURRENT;
> +	else if (sscanf(s, "%dx%d", &config->base.width,
> +				    &config->base.height) == 2)
> +		config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_MODE;
> +	else if (parse_modeline(s, &config->modeline) == 0)
> +		config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_MODELINE;
> +	else {
> +		weston_log("Invalid mode \"%s\" for output %s\n",
> +			   s, name);
> +		config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_PREFERRED;
> +	}
> +	free(s);

I would like this part shared between the backend and the compositor.
As I said, the parsing of modeline should be in the backend, but also 
what I would call “light modeline”, i.e. "width x height". That put the 
“technical” part into the “technical code”.
Then you have a separate setting for on/off(/current).

The configure_output function would return a boolean, which indicates 
whether or not to activate that output. The modeline would be passed as 
a string (config->moduline) and could be NULL.
Returning FALSE obviously means that all the configuration will be 
ignored (and thus, you can return early, keeping everything to NULL) and 
the meaning of TRUE depends on the modeline:
- if it is a well-formed modeline (or “light modeline”), use it;
- if it is malformed, fallback to NULL;
- if it is NULL, use the preferred setting.

If “current” is really a needed setting (I do not know the use case it 
was added for), we can just use a 3-values enum as the return value:
- OFF = 0
- PREFERRED = 1
- CURRENT = -1
which will change the meaning of a NULL modeline.


What do you think?
If we agree on that, I can make a patch for this (as a follow-up to 
squash before the push maybe).

Cheers,

-- 

Quentin “Sardem FF7” Glidic