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

[Pekka Paalanen]

On Wed, 9 Dec 2015 19:08:58 +0200
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

> 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. ;)

Hi,

yeah. I recall I advocated this struct ABI approach as the first
attempt in just getting libweston out the door the first time. Daniel
is right, but I wonder how much it matters at this point. Of course, it
would be nice that new additions are not something we already know
we're going to have to rewrite.

The good thing about structs is that they are type-safe at build time,
and there is no repeated text parsing, snprintf, parsing again. But
it's not too ABI friendly, while a key-value-list is. Let's see if I
could propose something to have both.

enum backend {
	DRM,
	FBDEV,
	X11
};

union option_value {
	int i;
	void *ptr;
	void (*func)(void);
	char *str;
};

struct backend_option {
	union option_value value;
	int option;
};

struct backend_option_list {
	int cur;
	int alloc;
	int backend;
	struct backend_option opts[0];
};

struct backend_option_list *
create_backend_option_list(int n, int backend)
{
	struct backend_option_list *list;

	list = calloc(1, sizeof(backend_option_list) + n * sizeof(backend_option));
	list->cur = 0;
	list->alloc = n;
	list->backend = backend;

	return list;
}

struct option_value *
backend_option_new(struct backend_option_list *list, int backend, int option)
{
	struct backend_option *opt = list->opts[list->cur++];

	if (backend != list->backend)
		abort();

	if (list->cur > list->alloc)
		abort();

	opt->option = option;

	return &opt->value;
}

That was the generic part. Then the DRM-specific part in the header:

enum drm_option {
	DRM_CONNECTOR,
	DRM_SEAT,
};

static inline void
drm_add_option_connector(struct backend_option_list *list, int connector)
{
	backend_option_new(list, DRM, DRM_CONNECTOR)->int = connector;
}

static inline void
drm_add_option_seat(struct backend_option_list *list, const char *seat)
{
	backend_option_new(list, DRM, DRM_SEAT)->str = strdup(seat);
}


And the it would be used in main.c like:

struct backend_options_list *opts;

opts = create_backend_option_list(5);
drm_add_option_connector(opts, 3);
drm_add_option_connector(opts, 4);
drm_add_option_seat(opts, "whee");

load_backend_new(compositor, "drm-backend.so", opts);


How's that for an idea? I admit I didn't think it all through.

All type-unsafe casting is in the inline functions. The library ABI
remains constant over adding new options and backends. New options just
add new enum values, which is an API addition, not an ABI break.

Although, an equally well working design would be to have a
backend-specific opaque config type, and then use functions to set
values in it, similar to above but not as inline functions in the
header. Hmm, but that would require libweston exporting lots of
backend-specific functions...

I didn't yet have time to properly review this patch, but regardless of
the ABI questions I am willing to give it at least:
Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Oh, and there's a compiler warning:

/home/pq/git/weston/src/compositor-drm.c: In function ‘create_output_for_connector’:
/home/pq/git/weston/src/compositor-drm.c:2283:9: warning: missing braces around initializer [-Wmissing-braces]
  struct weston_drm_backend_output_config config = { 0 };


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151211/45930580/attachment.sig>