On Fri, 11 Dec 2015 17:49:57 +0200 Giulio Camuffo <[email protected]> wrote:
> 2015-12-11 17:31 GMT+02:00 Pekka Paalanen <[email protected]>: > > On Wed, 9 Dec 2015 19:08:58 +0200 > > Giulio Camuffo <[email protected]> wrote: > > > >> 2015-12-09 18:58 GMT+02:00 Daniel Stone <[email protected]>: > >> > Hi, > >> > > >> > On 9 December 2015 at 16:29, Giulio Camuffo <[email protected]> > >> > 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. ... > > 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. > > I haven't really looked that carefully at it, but i must say i'm quite > tired of changing this patch over and over. I am willing to change it > again only if everybody relevant says that he's ok with the proposal, > otherwise it's just too much work for maybe no gain. Even better would > be to merge this for now, and to do a follow up later if we really > don't like the structs. Hi, like I said below, I'm ok landing your design. I am also almost certain it will need to be rewritten later. So if we go with this design now, all backends need to get migrated to this, and then later we'll migrate them all again. That's why I think bikeshedding the drm-backend patch into shape according to our best current knowledge now would be less work in total in the foreseeable future. But yes, I too would appreciate more opinions like you are calling for. My proposal is just what came to my mind one day, it's just an idea and not a requirement. > > 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 <[email protected]> > > > > 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
pgpAnmTiCfL5B.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
