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

[Giulio Camuffo]

2015-11-20 11:38 GMT+02:00 Quentin Glidic <sardemff7+wayland at 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