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

[Daniel Stone]

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.

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