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

[Giulio Camuffo]

2015-12-09 18:58 GMT+02:00 Daniel Stone <daniel at fooishbar.org>:
> Hi,
>
> On 9 December 2015 at 16:29, Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>> +enum weston_drm_backend_output_mode {
>> +       /** The output is disabled */
>> +       WESTON_DRM_BACKEND_OUTPUT_OFF,
>> +       /** The output will use the current active mode */
>> +       WESTON_DRM_BACKEND_OUTPUT_CURRENT,
>> +       /** The output will use the preferred mode. A modeline can be provided
>> +        * by setting weston_backend_output_config::modeline in the form of
>> +        * "WIDTHxHEIGHT" or in the form of an explicit modeline calculated
>> +        * using e.g. the cvt tool. If a valid modeline is supplied it will be
>> +        * used, if invalid or NULL the preferred available mode will be used. */
>> +       WESTON_DRM_BACKEND_OUTPUT_PREFERRED,
>> +};
>> +
>> +struct weston_drm_backend_output_config {
>> +       struct weston_backend_output_config base;
>> +
>> +       /** The pixel format to be used by the output. Valid values are:
>> +        * - NULL - The format set at backend creation time will be used;
>> +        * - "xrgb8888";
>> +        * - "rgb565"
>> +        * - "xrgb2101010"
>> +        */
>> +       char *format;
>> +       /** The seat to be used by the output. Set to NULL to use the
>> +        * default seat. */
>> +       char *seat;
>> +       /** The modeline to be used by the output. Refer to the documentation
>> +        * of WESTON_DRM_BACKEND_OUTPUT_PREFERRED for details. */
>> +       char *modeline;
>> +};
>> +
>> +/** The backend configuration struct.
>> + *
>> + * weston_drm_backend_config contains the configuration used by a DRM
>> + * backend. The backend will take ownership of the weston_backend_config
>> + * object passed to it on initialization and will free it on destruction. */
>> +struct weston_drm_backend_config {
>> +       struct weston_backend_config base;
>> +
>> +       /** The connector id of the output to be initialized. A value of 0 will
>> +        * enable all available outputs. */
>> +       int connector;
>> +       /** The tty to be used. Set to 0 to use the current tty. */
>> +       int tty;
>> +       /** If true the pixman renderer will be used instead of the OpenGL ES
>> +        * renderer. */
>> +       bool use_pixman;
>> +       /** The seat to be used for input and output. If NULL the default "seat0"
>> +        * will be used.
>> +        * The backend will take ownership of the seat_id pointer and will free
>> +        * it on backend destruction. */
>> +       char *seat_id;
>> +       /** The pixel format of the framebuffer to be used. Valid values are:
>> +        * - NULL - The default format ("xrgb8888") will be used;
>> +        * - "xrgb8888";
>> +        * - "rgb565"
>> +        * - "xrgb2101010"
>> +        * The backend will take ownership of the format pointer and will free
>> +        * it on backend destruction. */
>> +       char *format;
>> +
>> +       /** Callback used to configure the outputs. This function will be called
>> +        * by the backend when a new DRM output needs to be configured. */
>> +       enum weston_drm_backend_output_mode
>> +               (*configure_output)(struct weston_compositor *compositor,
>> +                                   struct weston_drm_backend_config *backend_config,
>> +                                   const char *name,
>> +                                   struct weston_drm_backend_output_config *output_config);
>> +};
>
> My main concern here is that encoding this as ABI (every single option
> for every single backend) seems a bit ambitious, and likely to lead to
> version churn. To avoid this, you could look at either some kind of
> generic key/value-store query API, or at least encoding a version
> number into the struct, so you don't have to incompatibly break ABI
> every time you add an option.

Well, but the plan is indeed to make multiple versions co-installable,
and anyway the API/ABI will change in every weston release at least in
the core so i don't think there is much point in going the extra mile
to make the ABI stable here, just yet.

>
> Other than that, looks fine enough to me, at a first pass, whilst
> being sick. So probably not quite strong enough for a Reviewed-by. ;)
>
> Cheers,
> Daniel