2015-11-20 11:38 GMT+02:00 Quentin Glidic <sardemff7+wayl...@sardemff7.net>:
> 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.
I'm not sure what current is for but i would not remove it.
>
>
> 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).
I think it sounds ok, but i would have to see how it turns up to be sure ;)
Thanks,
Giulio
>
> Cheers,
>
> --
>
> Quentin “Sardem FF7” Glidic
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
